ICM45686 AN-000364 FIFO Fix #97742
ICM45686 AN-000364 FIFO Fix #97742
Conversation
When an unsupported fifo packet triggers an assert, it is helpful to see the packet as a hex value to help when debugging. Signed-off-by: Anthony Williams <[email protected]>
anthony289478
changed the title
zephyrbot
requested review from
MaureenHelm,
afontaine-invn,
asemjonovs,
avisconti,
gjabouley-invn,
jeppenodgaard,
rbuisson-invn,
sriccardi-invn,
teburd,
tristan-google,
ubieda and
yperess
anthony289478
force-pushed
the
icm45686-an000364-fifo-fix
branch
from
1216db8 to
a79a687
Compare
When operating in streaming mode, a FIFO empty event, which is caused by host reading the last byte of the last FIFO frame, can cause FIFO data corruption. During subsequent reads to the FIFO, the first frame that arrives after the empty condition will be corrupted. Once the issue occurs, the internal state cannot recover and the FIFO must be flushed in bypass mode to clear the corrupted state. The current workaround from TDK is to read M-1 frames when M frames are reported by fifo_count. Since M is fixed by the fifo_watermark DT parameter, and in cases where fifo_watermark == 1, the watermark trigger threshold is set to M + 1 frames and M frames are read out during a watermark threshold event. Signed-off-by: Anthony Williams <[email protected]>
anthony289478
force-pushed
the
icm45686-an000364-fifo-fix
branch
from
a79a687 to
02b06bc
Compare
Change the __ASSERT on unsupported fifo packet header to CHECKIF so the user can choose how to handle the invalid packet. Signed-off-by: Anthony Williams <[email protected]>
The size of icm45686_encoded_data fifo_payload is guaranteed to span the full number of frames read as it was allocated during icm45686_event_handler() buf_len_required size. This update suppresses the static analysis warning of out of bounds memory access at index 1 by explicitly declaring fifo_payload to be a VLA at the end of icm45686_encoded_data. Signed-off-by: Anthony Williams <[email protected]>
|
dceeaef fixes the sonarqubecloud error
|
|
I don't mean to veer off the PR topic, but I wonder if we should add a zephyr/include/zephyr/drivers/sensor.h Line 485 in 052ded1 Feels like, even though this is typically encoded in the data, it'd be safer to not entirely rely on the data when deciding how far to go pass the edata address (e.g: a bunch of 0xFF's in a corrupted buffer). What do you think? @teburd @MaureenHelm @yperess |
|
I'll need to go back and double check, but I think there was a reason for that. I seem to recall that it was hard or useless to guarantee the size at the decode phase. I'm not opposed to it though. |
5 participants
When running the icm45686 on current main for several hours, occasionally I hit this assert,
zephyr/drivers/sensor/tdk/icm45686/icm45686_decoder.c
Lines 492 to 496 in 3e7a941
When inspecting the value of the header, it is 0xff which is invalid and lead me to this comment in the official TDK HAL driver.
zephyrproject-rtos/hal_tdk/icm456xx/imu/inv_imu_driver_advanced.c
Since the current icm45686 driver implementation reads the fixed watermark threshold number of frames, and in cases where the watermark is equal to 1, the workaround is the watermark threshold set to M + 1 frames and M frames are read out on each threshold trigger.