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

Fix LTM Scheduler not being set if port is shared. #3030

Merged

Conversation

giacomo892
Copy link
Collaborator

Thanks @fiam to pointing this out during tests to try to share SBUS port with LTM.

Seems like there are only a few useful usages related to sharing LTM with SBUS due to the nature of SBUS protocol.

@stronnag
Copy link
Collaborator

Doesn't this break the speed sanity check, allowing a configured value to override a speed limitation?

@giacomo892
Copy link
Collaborator Author

When LTM is shared with a Serial RX protocol the baud rate will be high enough. Like 100000b/s for SBUS and 115200b/s for IBUS for istance. Am I wrong?

@giacomo892 giacomo892 force-pushed the ltm_shared_scheduler_fix branch 2 times, most recently from 5e78a1e to e4f4c8b Compare April 11, 2018 13:05
@stronnag
Copy link
Collaborator

It may be shared with a MSP at 1200 baud. Does your change cover that case?

@stronnag
Copy link
Collaborator

Currently the code handles two cases:

  • Baud rate is adequate, but the user wants to set a slower rate (e.g. 9600 and slow LTM)
  • User has set a fast rate, but the baud rate is too low (1200 baud)

In both cases, the LTM rate 'does the right thing', in the latter case avoiding data overrun / corruption.

It would be helpful if this behaviour could be preserved for all use cases.

@giacomo892
Copy link
Collaborator Author

MSP and LTM on the same port is not really a "shared" port. Since LTM isn't working at the same time as MSP. LTM will be emitted only when armed. So everything should work as it was working before in my opinion.

@stronnag
Copy link
Collaborator

I don''t think so, in the latter case, the scheduler is set unconditionally from config in configureLtmScheduler, overriding the baud checks in configureLtmTelemetryPort(). Moving the baud based checks to after the configureLtmScheduler call would restore the expected behaviour in this case.

@giacomo892
Copy link
Collaborator Author

Fact is that port setup is not done when the port is shared with another protocol since the port has being already initialized by the other protocol.

@stronnag
Copy link
Collaborator

That is not the point, consider the case where the user forgets that the default for ltm_update_rate is NORMAL (300 bytes/sec) and starts with:

serial 2 17 9600 38400 9600 115200

Everything works, 300 bytes/sec LTM is supported by 9600 baud.
Then the user decidest o see if she can get much better range at a lower speed and sets:

serial 2 17 1200 38400 1200 115200

The current code will automatically switch the ltm_scheduler to SLOW based on the baud rate; 1200 can only support 100 bytes/sec. And everything magically still works.

With this PR, this case is broken, the ltm_schduler is always selected from config and not sanitised by the baud rate. This is an (unintended) regression. The 1200 baud pipe cannot support 300 bytes/sec. Telemetry is corrupted.

It is also easily fixed by changing:

  configureLtmTelemetryPort();
  configureLtmScheduler();

to

  configureLtmScheduler();
  configureLtmTelemetryPort();

then the previous, intended behaviour is maintained.

Copy link
Collaborator

@stronnag stronnag left a comment

Choose a reason for hiding this comment

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

as configureLtmScheduler() is only used by ltm.c, it can be declared before use as static and does not need to be forward declared in ltm.h

Please change the order of

configureLtmTelemetryPort();
configureLtmScheduler();

to

configureLtmScheduler();
configureLtmTelemetryPort();

in order to maintain the current baud rate sanitisation of ltm_schedule

@giacomo892 giacomo892 force-pushed the ltm_shared_scheduler_fix branch from e4f4c8b to 8b30841 Compare April 11, 2018 19:45
@giacomo892
Copy link
Collaborator Author

@stronnag Followed your advices. Thanks.

Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

Looks ok to merge to me

@digitalentity digitalentity added this to the 1.9.1 milestone Apr 17, 2018
@digitalentity digitalentity merged commit c9d0a98 into iNavFlight:development Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants