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: reset proto scan software timer to MULTI_PROTOLIST_START_TIMEOUT #3763

Merged
merged 1 commit into from Jul 10, 2023

Conversation

mha1
Copy link
Contributor

@mha1 mha1 commented Jul 5, 2023

... if timer already exists (from previous scan)

fixes #3084

changes:
Protocol scans use software timers to fetch the MPM protocol list at 100ms rate but with an initial rate of MULTI_PROTOLIST_START_TIMEOUT (3000ms) for the first poll to allow the MPM to finish its startup sequence after power on. Switching models back and forth power cycles the MPM. As the software timer already exists it is not instantiated again but reused. but with the rate it was left, i.e. 100ms, not the required 3000ms to allow the MPM to settle. Resetting the timer rate to MULTI_PROTOLIST_START_TIMEOUT solves the problem.

Note: the current implementation will under certain circumstances (e.g. model change back and forth) poll the MPM at 100ms with every poll failing and rebuilding the fallback list because the MPM missed the initial handshake. This goes on for the entire radio power life cycle. This is why I'd strongly advise to cherry pick the fix also into 2.8.5 and 2.9.

reset proto scan software timer to MULTI_PROTOLIST_START_TIMEOUT if timer already exists (from previous scan)
@pfeerick
Copy link
Member

pfeerick commented Jul 5, 2023

There's no need for the 2.8 / 2.9 ports unless there is an actual change needed for the different branches... git will cleanly cherrypick them regardless of line changes. This also has the benefit of cross-referencing a commit across different branches.

@pfeerick pfeerick added the bug 🪲 Something isn't working label Jul 5, 2023
@mha1
Copy link
Contributor Author

mha1 commented Jul 5, 2023

Yes, I know, but wanted to have 2.8.4+ built to ask the problem finder(s) to give it a go before messing up 2.8.5

@raphaelcoeffic
Copy link
Member

There's no need for the 2.8 / 2.9 ports

Plus, it is easier to track if everything is linked back to the same original PR.

@mha1
Copy link
Contributor Author

mha1 commented Jul 8, 2023

problem finder responded, ok. I'll close the 2.8 and 2.9 ports, cherry-picking main will work

@pfeerick
Copy link
Member

pfeerick commented Jul 9, 2023

Sorry, I should have commented further on this... I realised what Michael was up to later, as the issue was easiest reproduced on 2.8, as the MPM list is incomplete there. Plus, since no settings were changed, so it also let the original reporter test the fix without worrying about model/radio settings. So all good there.

Copy link
Member

@pfeerick pfeerick left a comment

Choose a reason for hiding this comment

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

Fixes the issue identified, haven't noticed any side effects. TX16S and Zorro (only as a double check since this should be a MULTI_PROTOLIST and hence colorlcd specific bug).

@pfeerick pfeerick added this to the 2.8.5 milestone Jul 9, 2023
@mha1
Copy link
Contributor Author

mha1 commented Jul 9, 2023

It shouldn't be color specific. Just depends on MPM internal or external.

@pfeerick
Copy link
Member

By "colorlcd specific bug" I'm referring to the fact that even though it is common code, you can't reproduce the bug on B&W ;)

@raphaelcoeffic
Copy link
Member

@mha1 @pfeerick AFAIK, this proto list isn't used on stdlcd. These targets use the info passed along with the telemetry for each protocol as it is switched on.

@pfeerick
Copy link
Member

pfeerick commented Jul 10, 2023

Yeah, I can't quite see the stepping code (eyes glazing over), but I thought B&W was still steered by the MPM. Anyway, it works 🤪 ... and you've not complained yet 😁

@mha1
Copy link
Contributor Author

mha1 commented Jul 10, 2023

@pfeerick @raphaelcoeffic you are right. stdlcd retrieves protocols by the MPM status (protocol prev, current, next) or if the status is invalid (e.g. MPM missing) by the builtin fallback list. The fallback list is up to date now (= MPM 1.3.3.20) but needs to be synced with upcoming MPM releases.

This issue really is a colorlcd specific one because only colorlcd is scanning and collecting protocols.

@pfeerick pfeerick merged commit ec5ab0b into EdgeTX:main Jul 10, 2023
34 checks passed
pfeerick pushed a commit that referenced this pull request Jul 10, 2023
…#3763)

reset proto scan software timer to MULTI_PROTOLIST_START_TIMEOUT if timer already exists (from previous scan)
pfeerick pushed a commit that referenced this pull request Jul 22, 2023
…#3763)

reset proto scan software timer to MULTI_PROTOLIST_START_TIMEOUT if timer already exists (from previous scan)
@pfeerick pfeerick linked an issue Aug 1, 2023 that may be closed by this pull request
1 task
@pfeerick pfeerick deleted the fix_#3084_fix_proto_scan branch October 1, 2023 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to select the ASSAN protocol in a 4in1 Radiomaster Tx Module Wireless trainer using DSM protocols
4 participants