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.

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

Prevent time comparison overflow in S.Port driver #3536

Merged
merged 1 commit into from Jul 8, 2018

Conversation

Projects
None yet
3 participants
@fiam
Copy link
Member

commented Jul 7, 2018

Would stop S.Port telemetry after ~36 minutes.
Fixes #3282

@fiam fiam added this to the 2.0 milestone Jul 7, 2018

@@ -538,7 +538,11 @@ void handleSmartPortTelemetry(void)
}
}

if (cmpTimeUs(micros(), lastTelemetryFrameReceivedUs) >= SMARTPORT_MIN_TELEMETRY_RESPONSE_DELAY_US) {
// XXX: Checking for lastTelemetryFrameReceivedUs == 0 is needed for S.Port, since payload will
// only be non-null (and hence, lastTelemetryFrameReceivedUs will be updated) when using FPort.

This comment has been minimized.

Copy link
@mikeller

mikeller Jul 7, 2018

Contributor

On closer inspection, this is actually wrong. Payload is pointing to the incoming payload for both SmartPort and FPort:
https://github.com/iNavFlight/inav/pull/3536/files#diff-5d6c27b6b68ea6ef24a6103f1bc2ced4L535

A correct fix would be to change

https://github.com/iNavFlight/inav/pull/3536/files#diff-5d6c27b6b68ea6ef24a6103f1bc2ced4R536

to

if (payload || clearToSend) {

Alternatively (and maybe even a bit cleaner), lastTelemetryFrameReceivedUs should be updated inside smartPortDataReceive().

Also, n.b. that handleSmartPortTelemetry() isn't run for FPort.

This comment has been minimized.

Copy link
@fiam

fiam Jul 7, 2018

Author Member

I thought about that, but clearToSend might reset lastTelemetryUs if we get an stray byte due to intererence. I wanted to fix it with minimal changes so it can go into inav 2.0

@fiam fiam force-pushed the agh_fix_smartport_time_overflow branch from a9183a5 to 8909396 Jul 7, 2018

@teckel12

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2018

👍

Prevent time comparison overflow in S.Port driver
Would stop S.Port telemetry after ~36 minutes.
After discussing this with @mikeller, we've decided to enforce the
500us response delay for S.Port, since it might be needed for some
receivers.

Fixes #3282

@fiam fiam force-pushed the agh_fix_smartport_time_overflow branch from 8909396 to 74e5ae8 Jul 8, 2018

@fiam fiam merged commit 6b2d554 into development Jul 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fiam fiam deleted the agh_fix_smartport_time_overflow branch Jul 8, 2018

@fiam fiam referenced this pull request Jul 8, 2018

Closed

Smartport telemetry #3282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.