I2c unsigned fw retry fix - #14107
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a typo in the I²C firmware retry mechanism so that the correct loop index (j) is used when deciding whether to sleep before retrying, enabling unsigned FW updates over I²C to succeed.
- Changed retry condition variable from
itojin the exception handler - Preserves sleep delay only on non-final retry attempts
Comments suppressed due to low confidence (1)
src/ds/ds-device-common.cpp:169
- Add or update unit tests covering the I²C flash backup retry logic, verifying both the sleep behavior on intermediate retries and the exception thrown on the final attempt.
catch (...)
| catch (...) | ||
| { | ||
| if (i < retries - 1) std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| if (j < retries - 1) std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
[nitpick] Add braces around the if and else branches to improve readability and reduce risk when modifying this block in the future.
| if (j < retries - 1) std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| if (j < retries - 1) { | |
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| } |
| } | ||
| catch (...) | ||
| { |
There was a problem hiding this comment.
[nitpick] Consider logging the retry attempt number (j) and the caught exception to aid debugging when an I²C flash backup retry fails.
| } | |
| catch (...) | |
| { | |
| } | |
| catch (const std::exception& e) | |
| { | |
| LOG_DEBUG("Retry attempt " << j + 1 << " failed with exception: " << e.what()); | |
| if (j < retries - 1) std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| else throw; | |
| } | |
| catch (...) | |
| { | |
| LOG_DEBUG("Retry attempt " << j + 1 << " failed with an unknown exception."); |
remibettan
left a comment
There was a problem hiding this comment.
Is the sleep really needed here?
@remibettan - Not sure, it was already there, but it happens only upon retry (which should happen up to 3 times) so I'm not sure it's an issue. |
So we have a backend retries and an app level retries.. |
|
@remibettan I think we need more investigation and work on unsigned FW update but this PR fix a BUG. |
sure |
ev-mp
left a comment
There was a problem hiding this comment.
100msec is huge in terms of i2c, and can make the flow run 2-4 times slower (which is already tens of sec). Is it possible to reduce it to [20-30]ms range? Alternatively/Additionally, if most of the fails are during the backup phase, then let's remove the FW backup from default flow, and make it available on demand via a dedicated API flag/attribute
The 100msec is only in case of retry which are observed between once and twice during the entire process so the impact should be very limited |
Tracked on [RSDSO-19992]
Fixed typo in flash backup retry mechanism.
Now unsigned FW update over I2C succeeds.