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: Internal PXX exclusive S.PORT #3822

Merged
merged 9 commits into from
Oct 12, 2023
Merged

fix: Internal PXX exclusive S.PORT #3822

merged 9 commits into from
Oct 12, 2023

Conversation

raphaelcoeffic
Copy link
Member

@raphaelcoeffic raphaelcoeffic commented Jul 18, 2023

This PR is meant to solve the issue caused by the shared S.PORT line between internal and external module on FrSky radios. The internal module on these radios take the S.PORT line in ownership when turned ON, and there is no way to turn it OFF.

Summary of Changes:

  • Force release S.PORT owned by the external module when a PXX1 internal is turned ON.
  • Re-Init external module (if powered ON) when a PXX1 internal module is turned OFF.
  • MPM: automatically send "disable telemetry" if S.PORT is not owned by the MPM.
  • PXX1 external module: automatically send "disable telemetry" if S.PORT is not owned by this module.
  • Fixes to improve conflict detection between different drivers using the same pins (USART / timers).

Please note: In case an external module uses S.PORT both for TX and RX (CRSF, GHOST), the initialisation of the internal module will fail. The UI should prevent this with the existing code.

PXX2 internal modules:

  • It seems that latest ISRM firmwares (tested with 2.1.9 EU on X10 Express) do not clog S.PORT anymore,
  • Express LRS / MPM seem to work normally in parallel with ISRM switched ON (both receivers connected).

@raphaelcoeffic raphaelcoeffic marked this pull request as draft July 18, 2023 14:25
@raphaelcoeffic raphaelcoeffic force-pushed the pxx-exclusive-sport branch 2 times, most recently from 50fc013 to c9b5033 Compare July 18, 2023 16:36
@3djc
Copy link
Collaborator

3djc commented Jul 19, 2023

Tested on X7 with internal XJT and external multi, each having a receiver connected to it:
what works:

  • individual (only one module on) module bind and use
  • both module at the same time (telemetry from receiver is logicaly received from XJT connected receivers)

what doesnt:
turn radio with both module on, turn off XJT, multi doesn't get back telemetry
turn radio with only MULTI on, turn on XJT, multi telem doesn't get cut and is corrupted
turn raido with both modules on, turn both module off, turn multi on, displays: no telemetry

@raphaelcoeffic
Copy link
Member Author

Ok, so I'll to check it out with real hardware. Maybe I can add some unit tests for MPM and see how that goes.

@raphaelcoeffic raphaelcoeffic force-pushed the pxx-exclusive-sport branch 2 times, most recently from 345bf43 to b040846 Compare August 11, 2023 05:08
@raphaelcoeffic raphaelcoeffic changed the title Draft: internal PXX exclusive S.PORT fix: internal PXX exclusive S.PORT Aug 12, 2023
@raphaelcoeffic raphaelcoeffic marked this pull request as ready for review September 29, 2023 06:39
@raphaelcoeffic raphaelcoeffic added this to the 2.10 milestone Sep 29, 2023
@pfeerick pfeerick force-pushed the pxx-exclusive-sport branch from b040846 to 964bd62 Compare October 1, 2023 05:26
@pfeerick pfeerick changed the title fix: internal PXX exclusive S.PORT fix: Internal PXX exclusive S.PORT Oct 3, 2023
@pfeerick
Copy link
Member

pfeerick commented Oct 7, 2023

I'm having trouble with the MPM here. Before this PR, with X9D+2019 (ISRM) + external MPM, I could use both MPM and ISRM simultaneously... just the MPM telemetry would get cut. As soon as I load this PR, I can not get the MPM telemetry up, regardless of whether the ISRM is on or off.

@raphaelcoeffic
Copy link
Member Author

As soon as I load this PR, I can not get the MPM telemetry up, regardless of whether the ISRM is on or off.

I'm having issues understanding this. This PR does not change PXX2 handling, nor should it change anything in the MPM behaviour in your specific case. There must be something big I missed here.

@pfeerick
Copy link
Member

I'll try this again today, and also double-check against main to make sure the issue isn't actually there.

@pfeerick
Copy link
Member

pfeerick commented Oct 12, 2023

Both nightly and the latest rebase are fine on X9D+2019 (ISRM) now. 🤷 I've also tried on the X9D+ now with this PR, and gone through all three of JCs failure cases and they are working for me. It's nice to see the prompt about disabling internal RF rather than just "no telemetry".

@pfeerick
Copy link
Member

pfeerick commented Oct 12, 2023

A further enhancement that would be worth considering (since you mentioned CRSF;GHOST) would be for it to be somehow blocked - whether unelectable or error msg shown - until internal RF is disabled (since it will effectively kill internal RF if you enable GHOST after the internal RF is enabled - which you can do still on the X9D+ - you just can't enable the internal RF after enabling GHST). The internal RF is also currently not re-enabled if it was originally active, and GHOST is then enabled. and disabled. I've not tried with CRSF, but assume it would be the same.

@pfeerick pfeerick added the enhancement ✨ New feature or request label Oct 12, 2023
richardclli added a commit that referenced this pull request Nov 2, 2023
richardclli added a commit that referenced this pull request Nov 2, 2023
richardclli added a commit that referenced this pull request Nov 5, 2023
pfeerick pushed a commit that referenced this pull request Nov 7, 2023
pfeerick pushed a commit that referenced this pull request Nov 7, 2023
pfeerick pushed a commit that referenced this pull request Nov 10, 2023
pfeerick pushed a commit that referenced this pull request Nov 12, 2023
pfeerick pushed a commit that referenced this pull request Nov 12, 2023
richardclli added a commit that referenced this pull request Dec 3, 2023
richardclli added a commit that referenced this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants