AP_ESC_Telem: Improve handling of time wrap with RPM data #23896
Conversation
WickedShell
force-pushed
the
wickedshell/esc-rpm-wrap-handling
branch
from
1cdd69f to
953a658
Compare
|
Initial bench testing shows RPM data is still working, disconnecting/connecting the telemetry lines leads to it coming back and I used the set start time code to ensure that nothing unexpected happened when wrapping 32 microseconds. |
WickedShell
force-pushed
the
wickedshell/esc-rpm-wrap-handling
branch
from
953a658 to
86f54db
Compare
|
@andyp1per I've significantly reworked this to add a flag as requested. I found that it made the various if statements doing the actual work really complicated though, so I inserted some helpers that force everything to have the same view. |
WickedShell
force-pushed
the
wickedshell/esc-rpm-wrap-handling
branch
from
86f54db to
a35966d
Compare
| @@ -529,9 +528,32 @@ void AP_ESC_Telem::update() | |||
| _last_rpm_log_us[i] = _rpm_data[i].last_update_us; | |||
| } | |||
| } | |||
|
|
|||
| if ((now_us - _rpm_data[i].last_update_us) > ESC_RPM_DATA_TIMEOUT_US) { | |||
There was a problem hiding this comment.
Isn't this rpm_data_within_timeout()?
There was a problem hiding this comment.
This is what's actually setting the flag for us, the helper checks the data valid flag, which seemed kinda confusing.
There was a problem hiding this comment.
Okay, I came back to this, it does work just fine, but it makes it 24 bytes larger. In practice you will still bail out at the same time in general, but it has to run 2 more checks then it otherwise would have if that's not the case. I'm inclined to leave it as I presented since this is the explicit wrap protection.
| return instance.data_valid; | ||
| } | ||
|
|
||
| bool AP_ESC_Telem::was_rpm_data_evers_reported (const volatile AP_ESC_Telem_Backend::RpmData &instance) const |
There was a problem hiding this comment.
was_rpm_data_ever_reported()?
not a great fan of passing in the backend. Since it can be static you could make it a member of the backend or just pass in last_update_us
There was a problem hiding this comment.
So the backend doesn't actually have the RpmData struct. It's declared back there, but it actually lives and exists in the frontend, which is why I stuck the helper there. I thought about passing the instance flag, but found that half the places were iterating over a reference anyways. I can stick it in the RpmData struct, but wasn't sure that actually seemed easier.
| } | ||
| } | ||
|
|
||
| bool AP_ESC_Telem::rpm_data_within_timeout (const volatile AP_ESC_Telem_Backend::RpmData &instance, const uint32_t now_us, const uint32_t timeout_us) const |
There was a problem hiding this comment.
could just pass in last_update_us and data_valid or make it a member of the backend
There was a problem hiding this comment.
Yeah, but at a certain point it gets a lot larger of a function call, and makes the calling sites a lot less clear. I'm inclined to not pass everything...
WickedShell
force-pushed
the
wickedshell/esc-rpm-wrap-handling
branch
3 times, most recently
from
b0da5de to
cb06bb0
Compare
…data if a ESC stops responding
|
Flown on a copter, with harmonic notch from ESC telemetry, no issues. |
The primary target here is to close a window where after time wraps we would ignore the last reported RPM data even if it was valid (such as in the are_motors_running function). The secondary thing this helps us with is that by zeroing out the time we can ensure that if we lose telemetry after wrapping around we don't have a window of time where we consider it to be valid again.
This PR is worth reviewing in separate commits. The second commit is a combination of me kibitzing, and attempting some drive by improvements. They're detailed in the commit message and should be reviewed carefully.
Thus far this has been run through the autotest sequence, but needs to be tested on real hardware still. I'm hoping to get back to that shortly, and do some explicit testing of the behavior on wraps.