Improve MSP handling #3415

haslinghuis · 2023-04-10T01:02:12Z

Give FC time to react to new settings
Reduce MSP.timeout from 250 to 50ms
Reduce timeout from 250 to 200ms
Add new mspHelper for EEPROM write

blckmn · 2023-04-10T02:17:04Z

AUTOMERGE: (FAIL)

github identifies PR as mergeable -> PASS
assigned to a milestone -> PASS
cooling off period lapsed -> PASS
commit count less or equal to three -> FAIL
Don't merge label NOT found -> PASS
at least one RN: label found -> PASS
Tested label found -> FAIL
assigned to an approver -> PASS
approver count at least three -> FAIL

HThuren · 2023-04-10T14:25:40Z

remove when already are ind queue.... but don't you risk, this case: have a long transaktion, put short status request, another long transaktion, you now want same short status since you still have the answer from first - this are with this PR skipped, and you only get an (old) answer to the short status request.
Better if you refresh the first request, ie, delete and place last in queue, but not if it is a write action ....
Job queues handling are complex to handle...

haslinghuis · 2023-04-10T16:26:18Z

Sure this needs a lot of testing. But see here how it was before: e87c0ca

McGiverGim · 2023-04-10T18:46:35Z

This code sends the callback? Now we have a MSP request and we inform when finished to the caller. With this PR is discarded, do we inform the caller too?

HThuren · 2023-04-10T19:57:55Z

This code sends the callback? Now we have a MSP request and we inform when finished to the caller. With this PR is discarded, do we inform the caller too?

PR !, you mean request ? or ...

haslinghuis · 2023-04-10T20:04:27Z

@McGiverGim good call, this was not handled in original code either. Added callbacks

McGiverGim · 2023-04-11T06:20:23Z

src/js/msp.js

+                if (callback_msp) {
+                    callback_msp();
+                }


I'm not too sure if this can have some unexpected result. Some pages call the MSP commands at certain order, for example I think remember the VTX tab. With this, if I'm not wrong, it can call the callback way before if it is at the queue. But I suppose this is very similar to the code actual, is that true? I think this works because at the end, only events executed in a timer manner are affected by this.
Another question, this takes into account the parameters to the MSP command? If the same command is called twice with different parameters the second one is ignored?

These are valid concerns.

I think for read kind of operations this wouldn't matter that much, because it was read and put into FC or whatever is needed. Though some things require arguments as you mentioned so if reading mutliple things with same command but different request then it would have stale data.

Another thing is putting the data into FC, not 100% of the flow, but would this change mean that if there are 2 MSP commands sending data to FC, only the first one would get executed? So there would be weird state where you'd click something it wouldn't work, then click again for it to be saved?

The code is reverting an experimental fix from @cTn-dev as the concerns are valid but it's how it was e87c0ca

// utilizing callback/timeout system for all commands
// dev version 0.57 code below got recently changed due to the fact that queueing same MSP codes was unsupported
// and was causing trouble while backup/restoring configurations
// watch out if the recent change create any inconsistencies and then adjust accordingly

It's blazing fast - but need to log and test real situations.

🤔 As alternative for dropping last command - if it exists already - we could replace the existing with the new one. For the aux tab had to introduce a timer to stop the objects interval allowing ongoing execution.

If I understand correctly how MSP2_SET_TEXT works, every command has to be accounted for based on it's payload so we can't discard any of the messages. I know this might be an exception, but if future apis are built in similar way then it might be hard to maintain.

Reverted the code to apply for MSP V1 requests only.

McGiverGim · 2023-04-13T07:55:58Z

Only as suggestion, now that you are looking at how to accelerate MSP commands, we have a MULTIPLE_MSP command, more info here: #1605
That can be used to send various commands at once. It is not being used because maybe it can produce some problems with very old FCs if the message is too big, but now that we have abandoned "some" of the backward compatibility, maybe we can try to test it.
Of course, for other PR, not here ;)

McGiverGim

Approved, but the MSP_MULTIPLE command that I've mentioned, maybe must be an exception too? It can be used to send different commands to the FC, so it must not be "removed" if there are others in the queue.

nerdCopter · 2023-04-13T13:10:19Z

9e265595
successfully flashed 2 FC's, both 4.4.1-release and 4.5-zulu
successfully configured 2 FC's (un-soldered)
will revisit when @McGiverGim 's concern is resolved.

haslinghuis · 2023-04-13T21:38:54Z

Firmware buffer size overflow is solved by adding multiple MSP_MULTIPLE_MSP requests so this should certainly not be replaced if not processed before the new request.

MSP_MULTIPLE_MSP (would call it MSP_BATCH) has yet no examples for implementation. #1605

🤔 But as the code already allows an ongoing process to continue adding the second command should already work.
🤔 we should try parallel execution as MSP request should be independent on each other by design.

#2356 (comment)

McGiverGim · 2023-04-14T13:13:52Z

MSP_MULTIPLE_MSP (would call it MSP_BATCH) has yet no examples for implementation. #1605

There is one example in the text of the PR. I don't know if it's this what you want.

haslinghuis · 2023-05-02T20:53:28Z

Needed to refactor ports tab not using async as settings were not applied.

@McGiverGim @nerdCopter not so sure about replacing duplicate MSP requests so dropped that for the moment as clicking other tabs sometimes hung up loading sequence without notice - using a timeout was working but don't want to add another timer as this would still allow duplicates to run.

Speeding up communication works good by improving timing.

nerdCopter · 2023-05-02T20:57:39Z

i have it installed as my daily-driver, will keep an eye on it. (3e5d6442)

nerdCopter

~~approving. no known problems.~~
MSP timeouts, but as expected.

EDIT: dismissed

nerdCopter · 2023-05-06T15:36:08Z

SORRY, converted PR to draft to prevent merging. setting DSHOTXXX does not save to FC.
(3e5d6442)

https://youtu.be/nMeiG66xaPE

sonarcloud · 2023-05-06T16:17:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2023-05-06T16:31:40Z

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

nerdCopter

re-approving
c1b1e1a9 fixes last concern.

haslinghuis added the RN: IMPROVEMENT label Apr 10, 2023

haslinghuis added this to the 10.10.0 milestone Apr 10, 2023

haslinghuis self-assigned this Apr 10, 2023

haslinghuis force-pushed the bad-cable-detection branch from 6af758f to 1f1031b Compare April 10, 2023 01:04

This comment has been minimized.

Sign in to view

haslinghuis force-pushed the bad-cable-detection branch from 1f1031b to a4d876d Compare April 10, 2023 20:17

This comment has been minimized.

Sign in to view

McGiverGim reviewed Apr 11, 2023

View reviewed changes

haslinghuis force-pushed the bad-cable-detection branch from bef6821 to adf7a9f Compare April 11, 2023 23:30

This comment has been minimized.

Sign in to view

haslinghuis force-pushed the bad-cable-detection branch from 9894b2b to 6edba4d Compare April 12, 2023 21:12

haslinghuis changed the title ~~Remove duplicate MSP requests~~ Remove duplicate MSP V1 requests Apr 12, 2023

This comment has been minimized.

Sign in to view

haslinghuis force-pushed the bad-cable-detection branch 5 times, most recently from 25e7ccc to 9e26559 Compare April 12, 2023 21:36

This comment has been minimized.

Sign in to view

McGiverGim approved these changes Apr 13, 2023

View reviewed changes

haslinghuis added 5 commits May 2, 2023 00:51

Implement MULTIPLE_MSP

8f364ba

Revert

132a0d5

Give FC time to react to new settings

f539269

Reduce timeout to 200ms

72c7b53

Add new mspHelper for EEPROM write

9663574

haslinghuis force-pushed the bad-cable-detection branch from 091d7fe to 7de6e40 Compare May 1, 2023 22:54

haslinghuis removed the Don't merge label May 1, 2023

haslinghuis force-pushed the bad-cable-detection branch from 7de6e40 to 6b73572 Compare May 1, 2023 22:57

This comment has been minimized.

Sign in to view

haslinghuis force-pushed the bad-cable-detection branch from 6b73572 to 90a552c Compare May 2, 2023 00:43

This comment has been minimized.

Sign in to view

haslinghuis force-pushed the bad-cable-detection branch 2 times, most recently from dfd787e to 3e5d644 Compare May 2, 2023 00:59

haslinghuis changed the title ~~Remove duplicate MSP V1 requests~~ Improve MSP handling May 2, 2023

This comment has been minimized.

Sign in to view

nerdCopter approved these changes May 6, 2023

View reviewed changes

nerdCopter self-requested a review May 6, 2023 15:33

nerdCopter marked this pull request as draft May 6, 2023 15:35

haslinghuis force-pushed the bad-cable-detection branch from 3e5d644 to 3f1a5f3 Compare May 6, 2023 16:15

Reduce MIN_TIMEOUT

c1b1e1a

haslinghuis force-pushed the bad-cable-detection branch from 3f1a5f3 to c1b1e1a Compare May 6, 2023 16:16

nerdCopter approved these changes May 6, 2023

View reviewed changes

nerdCopter marked this pull request as ready for review May 6, 2023 16:47

haslinghuis merged commit 07a352a into betaflight:master May 6, 2023
9 checks passed

haslinghuis deleted the bad-cable-detection branch May 6, 2023 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve MSP handling #3415

Improve MSP handling #3415

haslinghuis commented Apr 10, 2023 •

edited

This comment has been minimized.

blckmn commented Apr 10, 2023 •

edited

HThuren commented Apr 10, 2023

haslinghuis commented Apr 10, 2023

McGiverGim commented Apr 10, 2023

HThuren commented Apr 10, 2023

haslinghuis commented Apr 10, 2023 •

edited

This comment has been minimized.

McGiverGim Apr 11, 2023

chmelevskij Apr 11, 2023

haslinghuis Apr 11, 2023 •

edited

chmelevskij Apr 12, 2023

haslinghuis Apr 12, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

McGiverGim commented Apr 13, 2023

McGiverGim left a comment •

edited

nerdCopter commented Apr 13, 2023

haslinghuis commented Apr 13, 2023 •

edited

McGiverGim commented Apr 14, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

haslinghuis commented May 2, 2023

nerdCopter commented May 2, 2023 •

edited

nerdCopter left a comment •

edited

nerdCopter commented May 6, 2023 •

edited

sonarcloud bot commented May 6, 2023

github-actions bot commented May 6, 2023

nerdCopter left a comment

Improve MSP handling #3415

Improve MSP handling #3415

Conversation

haslinghuis commented Apr 10, 2023 • edited

This comment has been minimized.

blckmn commented Apr 10, 2023 • edited

HThuren commented Apr 10, 2023

haslinghuis commented Apr 10, 2023

McGiverGim commented Apr 10, 2023

HThuren commented Apr 10, 2023

haslinghuis commented Apr 10, 2023 • edited

This comment has been minimized.

McGiverGim Apr 11, 2023

Choose a reason for hiding this comment

chmelevskij Apr 11, 2023

Choose a reason for hiding this comment

haslinghuis Apr 11, 2023 • edited

Choose a reason for hiding this comment

chmelevskij Apr 12, 2023

Choose a reason for hiding this comment

haslinghuis Apr 12, 2023

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

McGiverGim commented Apr 13, 2023

McGiverGim left a comment • edited

Choose a reason for hiding this comment

nerdCopter commented Apr 13, 2023

haslinghuis commented Apr 13, 2023 • edited

McGiverGim commented Apr 14, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

haslinghuis commented May 2, 2023

nerdCopter commented May 2, 2023 • edited

nerdCopter left a comment • edited

Choose a reason for hiding this comment

nerdCopter commented May 6, 2023 • edited

sonarcloud bot commented May 6, 2023

github-actions bot commented May 6, 2023

nerdCopter left a comment

Choose a reason for hiding this comment

haslinghuis commented Apr 10, 2023 •

edited

blckmn commented Apr 10, 2023 •

edited

haslinghuis commented Apr 10, 2023 •

edited

haslinghuis Apr 11, 2023 •

edited

McGiverGim left a comment •

edited

haslinghuis commented Apr 13, 2023 •

edited

nerdCopter commented May 2, 2023 •

edited

nerdCopter left a comment •

edited

nerdCopter commented May 6, 2023 •

edited