Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_ESC_Telem: fix RPM timeout race #28987

Closed
wants to merge 1 commit into from
Closed

AP_ESC_Telem: fix RPM timeout race #28987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions libraries/AP_ESC_Telem/AP_ESC_Telem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

//#define ESC_TELEM_DEBUG

#define ESC_RPM_CHECK_TIMEOUT_US 210000UL // timeout for motor running validity
#define ESC_RPM_CHECK_TIMEOUT_US 210000L // timeout for motor running validity

extern const AP_HAL::HAL& hal;

Expand Down Expand Up @@ -202,7 +202,7 @@ bool AP_ESC_Telem::get_rpm(uint8_t esc_index, float& rpm) const

const uint32_t now = AP_HAL::micros();
if (rpm_data_within_timeout(rpmdata, now, ESC_RPM_DATA_TIMEOUT_US)) {
const float slew = MIN(1.0f, (now - rpmdata.last_update_us) * rpmdata.update_rate_hz * (1.0f / 1e6f));
const float slew = constrain_float(int32_t(now - rpmdata.last_update_us) * rpmdata.update_rate_hz * (1.0f / 1e6f), 0, 1);
rpm = (rpmdata.prev_rpm + (rpmdata.rpm - rpmdata.prev_rpm) * slew);

#if AP_SCRIPTING_ENABLED
Expand Down Expand Up @@ -811,16 +811,18 @@ void AP_ESC_Telem::update()
const uint32_t now_us = AP_HAL::micros();
for (uint8_t i = 0; i < ESC_TELEM_MAX_ESCS; i++) {
// Invalidate RPM data if not received for too long
if ((now_us - _rpm_data[i].last_update_us) > ESC_RPM_DATA_TIMEOUT_US) {
// (last_update_us might be fresher than now_us, so we use a signed difference)
if (int32_t(now_us - _rpm_data[i].last_update_us) > ESC_RPM_DATA_TIMEOUT_US) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don't think this works, because the same comparison happens at the wrap point, and it needs to register a timeout.

Copy link
Collaborator Author

@robertlong13 robertlong13 Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about when the time difference wraps around? After the difference wraps around (after 36 minutes being stale), this comparison stops setting data_valid to false, but that's okay because it has been pointlessly re-registering the timeout for 36 minutes and now it just stops setting it to false; it doesn't start setting it to true. And nothing sets it true until fresh data arrives.

Or are you talking about the time when now_us itself wraps around? Because that also works fine. Integer differences pretty much work fine with wraparound as long as both are the same type.

_rpm_data[i].data_valid = false;
}
}
}

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)
bool AP_ESC_Telem::rpm_data_within_timeout(const volatile AP_ESC_Telem_Backend::RpmData &instance, const uint32_t now_us, const int32_t timeout_us)
{
// easy case, has the time window been crossed so it's invalid
if ((now_us - instance.last_update_us) > timeout_us) {
// (last_update_us might be fresher than now_us, so we use a signed difference)
if (int32_t(now_us - instance.last_update_us) > timeout_us) {
return false;
}
// we never got a valid data, to it's invalid
Expand Down
4 changes: 2 additions & 2 deletions libraries/AP_ESC_Telem/AP_ESC_Telem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
static_assert(ESC_TELEM_MAX_ESCS > 0, "Cannot have 0 ESC telemetry instances");

#define ESC_TELEM_DATA_TIMEOUT_MS 5000UL
#define ESC_RPM_DATA_TIMEOUT_US 1000000UL
#define ESC_RPM_DATA_TIMEOUT_US 1000000L

class AP_ESC_Telem {
public:
Expand Down Expand Up @@ -134,7 +134,7 @@ class AP_ESC_Telem {
private:

// helper that validates RPM data
static bool rpm_data_within_timeout (const volatile AP_ESC_Telem_Backend::RpmData &instance, const uint32_t now_us, const uint32_t timeout_us);
static bool rpm_data_within_timeout (const volatile AP_ESC_Telem_Backend::RpmData &instance, const uint32_t now_us, const int32_t timeout_us);
static bool was_rpm_data_ever_reported (const volatile AP_ESC_Telem_Backend::RpmData &instance);

#if AP_EXTENDED_DSHOT_TELEM_V2_ENABLED
Expand Down
Loading