sensor: adxl345: fix broken FIFO bypass mode #85635
Conversation
|
Hello @Rubusch, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
MaureenHelm,
asemjonovs,
avisconti,
galak,
jeppenodgaard,
microbuilder,
ozersa,
teburd,
tristan-google,
ttmut,
ubieda,
yasinustunerg and
yperess
|
Hi all, |
|
@dimitrije-lilic thank you for answering. I know this is a bit "pushy" now. Would you mind to approve to this pull request, or if reject, please let me know what I could improve? :) I mean, these are just 3 very limited commits.. I could come up with something more thorough in a follow up. But I did not want to interfere with someone's work. So, if you (or any of you guys) already planned to do a general brush up to this ADXL345 zephyr driver? |
drivers/sensor/adi/adxl345/adxl345.c
Outdated
| if (data->fifo_config.fifo_mode == ADXL345_FIFO_BYPASSED) { | ||
| rc = adxl345_read_sample(dev, &sample); | ||
| if (rc < 0) { | ||
| LOG_ERR("Failed to read sample rc=%d", rc); | ||
| return rc; | ||
| } | ||
|
|
||
| data->bufx[0] = sample.x; | ||
| data->bufy[0] = sample.y; | ||
| data->bufz[0] = sample.z; | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
I think it would be more intuitive to conditionally read samples_count if in FIFO mode, otherwise set samples_count to 1. Then proceed with the existing adxl345_read_sample logic.
There was a problem hiding this comment.
Hi @MaureenHelm - Thank you so much! This Feedback helps. I will have a look and resubmit until, say, mid march (I'm currently travelling, and mostly offline).
There was a problem hiding this comment.
Ok, I changed this a bit. Note, I'm reading samples_count from FIFO status register for all other modes than focus would move rather to the source default fallback is not 1 for BYPASSED. I left a comment on that. FIFO bypassed is the only mode actually bypassing the FIFO, i.e. not having a FIFO, neither FIFO events and thus actually won't have FIFO status register entries do evaluate, such as sample_count.
drivers/sensor/adi/adxl345/adxl345.c
Outdated
| return -EIO; | ||
| } | ||
|
|
||
| #ifdef CONFIG_ADXL345_TRIGGER |
There was a problem hiding this comment.
I agree this should not be hardcoded, remove the whole ifdef, suggested code:
rc = adxl345_configure_fifo(dev, cfg->fifo_config.fifo_mode, cfg->fifo_config.fifo_trigger, cfg->fifo_config.fifo_samples);
Also in the config default mode should be:
#define ADXL345_CONFIG(inst)
.fifo_config.fifo_mode = ADXL345_FIFO_BYPASSED, \
There was a problem hiding this comment.
Hi @dimitrije-lilic - first of all, thank you so much for this feedback! I really appreciate, and updated the stance. Really good advice, thanks again. Pls, if you can find some time, have a look at it.
There was a problem hiding this comment.
Just a note, I separated setting the default fifo_mode to a separate commit. IMHO the original patch series was supposed to fix bypass mode more or less. Changing what is the default fifo mode is not quite the same, thus separate patch. If you guys disagree, pls let me know.
drivers/sensor/adi/adxl345/adxl345.c
Outdated
| struct adxl345_dev_data *data = dev->data; | ||
| uint8_t dev_id; | ||
| bool is_full_res = true; | ||
| #ifdef CONFIG_ADXL345_TRIGGER |
There was a problem hiding this comment.
remove ifdef just leave config because it is going to be used now
Rubusch
force-pushed
the
lothar/adxl345-v0.02
branch
from
b6e5f7e to
712f14e
Compare
|
[I'm unsure if this is the right place to discuss this, or better switch to discord on sensors / adi, pls let me know?] Stupid Question: Why not making all the I see advantages in...
I'm unsure if this makes sense in this case for Zephyr. I can see, that DTS shall describe hardware, but TBH a FIFO mode is pretty fixed to the setup, and would not change. If the interrupt is wired, and the sensor supposed to work in STREAM mode, it would definitely be describing hardware, thus a case for DTS. |
I think the answer is related to downstream users. Imagine that you had a board with a sensor and you configured all the DTS attributes to add the interrupt lines in upstream Zephyr. But a downstream application doesn't want to use streaming APIs. Instead of having to modify the devicetree node in an overlay, they can just disable the Kconfig. |
There was a problem hiding this comment.
Overall this driver breaks the sensor API and should really just not support FIFO mode without the ASYNC API. fetch/get are meant to only support the "latest sample".
At a minimum, lets fix the initialization so it's using a devicetree binding enum instead of a kconfig which is a global value.
At best, this driver should implement the RTIO submit call for the FIFO.
drivers/sensor/adi/adxl345/adxl345.c
Outdated
| if (IS_ENABLED(CONFIG_ADXL345_TRIGGER)) { | ||
| fifo_mode = ADXL345_FIFO_TRIGGERED; | ||
| } else if (IS_ENABLED(CONFIG_ADXL345_STREAM)) { | ||
| fifo_mode = ADXL345_FIFO_STREAMED; | ||
|
|
||
| rc = adxl345_reg_write_byte(dev, ADXL345_FIFO_CTL_REG, | ||
| ADXL345_FIFO_STREAM_MODE); | ||
| if (rc < 0) { | ||
| LOG_ERR("FIFO enable failed\n"); | ||
| return -EIO; | ||
| } | ||
| } |
There was a problem hiding this comment.
This isn't good, if you had 2 of these sensors attached and you wanted one to use the streaming mode and the other bypass you'd end up with both using streaming. You should add to the dts binding an enum for fifo-mode: [bypass, trigger, stream] and save the devicetree value in the config. Then use that in the init function.
There was a problem hiding this comment.
If I understand you correctly this supports actually my idea I tried also to explain before i.e. to have the FIFO mode selection rather entirely over DTS, and remove the Kconfig related options. TBH I did not think of several sensors connected, but this is probably the strongest argument for DT binding.
I'll prepare the following TODOs
- FIFO mode selection entirely over DT binding
- RTIO submit call for the FIFO
There was a problem hiding this comment.
Hi, some days have passed. So far I can come up with an approach implementing TRIGGER mode based on a defined
GPIO line #1 or #2 in the DTS, and dealing with data ready, watermark and overrun events.
It will then set up Analog's "FIFO_STREAM" mode, and allow for optional registering callbacks for data ready,
watermark and/or overrun events. If none are provided, the FIFO is flushed to recover for re-generating new
interrupt events. Also, TRIGGER mode w/o defined GPIO lines is possible. Then it falls back to Analog's
"FIFO BYPASSED" mode. This is also the default mode if TRIGGER / ASYNC API is not configured.
RTIO is still a topic for me. It works +/- but I'd like to exclude it for now, and push this later. I'm still
around fixing and need a better understanding of it to be confident.
Basically I'm unsure if the pull request is still open, or if someone already took care of it. If still
open as it looks to me by current git history, I'd love to clean up, split into reasonable commits, check them
and and I will probably push an update within the next days.
Rubusch
force-pushed
the
lothar/adxl345-v0.02
branch
2 times, most recently
from
0cb1805 to
d1edd74
Compare
|
The current approach implements fetch & get with TRIGGER support to operate the sensor FIFO. One of two GPIO lines is configurable in the DTS now. The driver will map the according interrupt line for the sensor events. FIFO related sensor events are data ready, watermark and overrun. Each event allows for registering a handler from the application. The TRIGGER mechanism catches the sensor interrupts and also can flush the FIFO and reset the interrupt status register. The FIFO watermark (as also before the ODR) is configurable from an app by the set attr function. The FIFO works either in FIFO_BYPASS mode, when no zephyr TRIGGER (or STREAM) is configured, or when TRIGGER is configured, but no GPIO lines are defined in the DTS. If zephyr's TRIGGER is configured, the FIFO will operate in Analogs FIFO_STREAM mode [Note, there is Analog's FIFO_TRIGGER mode and there is TRIGGER in a zephyr context, I try to make it a bit clear here, I'm not talking about Analog's FIFO_TRIGGER mode, which is not implemented]. Since I need some feedback here, I will present it now. The next step then would be to bring the CONFIG_ADXL345_STREAM implementation based on RTIO up. I have some POC setup here, but it's not as stable. That's why I present still first the older fetch & get update. Note, I tried to organize the patches/commits a bit into the following order:
Please take this commit series as proposal. Each commit is checkpatch'd and verified compiling w/ and w/o TRIGGER config enabled. The final state verified working in all modes (STREAM/RTIO is not worse than before). Working for several weeks on the code easily results in renamings and formatting. To me it makes all sense, let me know if this is acceptable or still needs to be stripped out. |
Reorganize driver source. Regroup defines and format according to checkpatch. Remove ';' from macros, in case put parens according to checkpatch.pl, fix indention and long lines. This is a preparatory step for upcomming implementation. Signed-off-by: Lothar Rubusch <[email protected]>
Rename defines to make it more comprehensible. Enhance consistency and remove duplicates. Clearly distinguish if using an ADXL345_REG_ i.e. the register, or just a single bit inside a register. Apply bitfield macros, such as BIT(), GENMASK(), FIELD_GET(), etc. to factor out plain logic operations. Help out readability and maintainability. This step is a preparation for upcoming implementation on this sensor driver. Signed-off-by: Lothar Rubusch <[email protected]>
Rename function and make it more comprehensive and consistent. Since similar functions are named rather "update bits" e.g. in regmap under linux. Keeping similar names, might increase readability. Signed-off-by: Lothar Rubusch <[email protected]>
Remove the function since it is not used. Signed-off-by: Lothar Rubusch <[email protected]>
Use enum for selected_range as a refactoring. Signed-off-by: Lothar Rubusch <[email protected]>
Convert the type of is_full_range to a bool, since it is used as bool. Signed-off-by: Lothar Rubusch <[email protected]>
Rename it to make it more consistent with other Analog accelerometer drivers in zephyr. Make the type packed, since it is used as bitfield. Signed-off-by: Lothar Rubusch <[email protected]>
Change type of fifo_samples to unit8_t since the entries counter will cover in 6 bit, and we will use this counter for fifo lines or sample number, not for byte size. Thus it fits in 8 bit. Signed-off-by: Lothar Rubusch <[email protected]>
Simplify function and remove called subfunction to increase readability. Signed-off-by: Lothar Rubusch <[email protected]>
Simplify decoder implementation to increase readability and maintainability. Signed-off-by: Lothar Rubusch <[email protected]>
Bring in configuration of interrupt line. Let the DTS binding allow to configure INT1 or INT2. All interrupt based sensor events will go over the configured interrupt line. Signed-off-by: Lothar Rubusch <[email protected]>
Add INT1 and INT2 as optional interrupt line. When set, the mapping will go over one of both. Signed-off-by: Lothar Rubusch <[email protected]>
Update init, reset and prepare the sensor to work with or without the configured interrupt lines. The sensor defaults to FIFO BYPASS mode, when TRIGGER is configured and interrupt lines are set up in the DTS, it will put it into FIFO STREAM mode. Signed-off-by: Lothar Rubusch <[email protected]>
Update the implementation of fetch and get. Default to FIFO BYPASS mode. When GPIO lines are defined in the DT and CONFIG_ADXL345_TRIGGER is set, Analog's FIFO BYPASS mode is configured. In this case up to watermark samples are stored in the FIFO then a watermark event is triggered. If samples are not read out, an overrun event can be configured. Aside, also data ready event can be sent out by the sensor. Signed-off-by: Lothar Rubusch <[email protected]>
Flush the FIFO by reading out its elements. Where switching FIFO CTL mode would also remove the FIFO entries, it would leave the status register with a triggered data ready, watermark and/or overrun bit set. By datasheet only read out of the fifo will reset those flags again. Signed-off-by: Lothar Rubusch <[email protected]>
Make the watermark size of the fifo configurable as watermark. Signed-off-by: Lothar Rubusch <[email protected]>
Let the application API register handlers for data ready, watermark and overrun. The implementation so far covers the fetch & get approach. Signed-off-by: Lothar Rubusch <[email protected]>
Refactor the rtio implementation as preparatory step for the read and decode implementation. Signed-off-by: Lothar Rubusch <[email protected]>
Rubusch
force-pushed
the
lothar/adxl345-v0.02
branch
from
d1edd74 to
1a07f14
Compare
|
Hi all. While still waiting on feedback (since March), I noticed other patches to this driver went through. No problem, I can rebase, but... @ubieda : I noticed, you seem to be (still?) working on this. Are my changes interfering your current work? @MaureenHelm @yperess @dimitrije-lilic Did I miss something? The pullrequest was initially blocked for several weeks by a change request. Then suddenly the CR disappeared/was closed? and I needed to do rebase for those patches. Still waiting. Will I still get a feedback? Or is this dropped? Actually I thought of the patches just as a first step, followup steps still pending. Shall I open a different pull request? Shall I split somehow? Would be nice to see some feedback here. TIA |
|
@Rubusch First of all, apologies as I have not been closely following this PR. Given this driver has changed significantly since Feburary, would you mind test-driving it once again and pointing out what features are not working? If some of the issues you've found are still valid, I suggest rebasing this PR with thel atest changes. It seems both Streaming and Trigger mode should be working, as well as polled reads with Read/Decode API. References:
Thanks for your initiative! I'm sure we'll find something to get your first contribution in. |
|
@ubieda Great work! I saw the driver made huge progress and the worst bugs seemed to be fixed now. I guess I should have better checked before how active you guys were already working on this. Well, I was just playing with this sensor, so when I saw the driver did not work, just fixed it somehow. At least, I saw some of your fixes were the things I'd also liked to do, so I started on the right track already, yeah! Further, I learned I should have split up my stuff a bit more. Also, extending this initial 3 patches into almost 20 patches, is probably in github way better a close and a new open. Quite a difference to mailing lists. Interesting. Lessons learned!! ;) Nevermind. I'll close this MR. Perhaps I'll extract some single topics of this patch set, rebase, and prepare smaller individual patch sets. It's probably easier to discuss / accept / deny small individual patches right away. Note, skimming over the current state: Thank you so much to all who were involved and to ubieda for coming back to this and letting me know. No problem!! |
7 participants
Hello Zepherinos!
The ADXL345 accelerometer sensor was not showing up any values, if there was no trigger with FIFO configured. In such case the sensor falls back to FIFO bypass mode and usually still shows measurements, but does not provide events (data ready, fifo, single tap, double tap, free fall, activity/inactivy). The patch fixes the issue, and makes the sensor interface show data again when no trigger is configure.
Since this is my first zephyr series, I hope this matches the procedure to submit and contribute to the project. Please, don't hesitate to let me know if anything is missing here.
Signed-off-by: Lothar Rubusch [email protected]