-
-
Notifications
You must be signed in to change notification settings - Fork 895
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
Arrange configuration and motor tab #2356
Arrange configuration and motor tab #2356
Conversation
bdd6c35
to
048b3a6
Compare
src/js/tabs/servos.js
Outdated
Promise | ||
.resolve(true) | ||
.then(() => MSP.promise(MSPCodes.MSP_SERVO_CONFIGURATIONS)) | ||
.then(() => MSP.promise(MSPCodes.MSP_SERVO_MIX_RULES)) | ||
.then(() => MSP.promise(MSPCodes.MSP_RC)) | ||
.then(() => MSP.promise(MSPCodes.MSP_BOXNAMES)) | ||
.then(load_html) | ||
.catch((error) => console.log(error.message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be run in parallel or they can be in sequence?
If we make initialise
async we could do something like for the sequential
try {
await MSP.promise(MSPCodes.MSP_SERVO_CONFIGURATIONS);
await MSP.promise(MSPCodes.MSP_SERVO_MIX_RULES);
await MSP.promise(MSPCodes.MSP_RC);
await MSP.promise(MSPCodes.MSP_BOXNAMES);
load_html();
} catch(error) {
console.log(error)
}
Or for parallel
try {
await Promise.all([
MSP.promise(MSPCodes.MSP_SERVO_CONFIGURATIONS),
MSP.promise(MSPCodes.MSP_SERVO_MIX_RULES),
MSP.promise(MSPCodes.MSP_RC),
MSP.promise(MSPCodes.MSP_BOXNAMES),
])
load_html();
} catch(error) {
console.log(error)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no particular sequence as they don't depend on each other. So the first solution would work but I'm in doubt in what advantage the parallel solution has over the former.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chmelevskij - it's working 😜 Updated configuration.js
, motors.js
and servos.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If id doesn't matter about the sequence I'd use Promise.all
, bunch of await
s will block and wait for the previous to finish so you might get a bit of a lag if execution time adds up to 1s or smth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added some time ago one MSP command to execute various MSP commands at once. I don't remember the ID but can be useful for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it: #1605
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSP commands can definitely not be run in parallel - the firmware and the protocol lack the means to sequentialise / recombine responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for MSP_MULTIPLE_MSP
, I'd be reluctant to use this as a wrapper to group multiple commands in configurator - this will break and require another rewrite as soon as one of the combined frames (request or response) exceeds 255 bytes in length, which will happen sooner or later due to the MSP protocol's nature of having commands grow in lenght with every change.
17b1cf6
to
c126e58
Compare
@haslinghuis: Seeing how much change is still going on in this pull request, can you please change this to 'draft' status so reviewers can hold off until it is ready for review? |
c126e58
to
7307de7
Compare
6511d53
to
bc9f42a
Compare
@haslinghuis did you see my comment about MSP_MULTIPLE_MSP above? |
@McGiverGim yes but there is no example how to call it. And I was very very busy trying to fix the servos test. Motor tab and configuration tab are ready and @asizon offered to look at servos because I am on dead trail with it. |
d392dac
to
61e883e
Compare
Thanks @McGiverGim - your review was very helpfull. Addressed your points and it should be good to go now: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but as I said, others will not think the same :)
61e883e
to
55138e1
Compare
Minor improvement on width in motor testing (E values were too condensed on a smaller screen) and moved @limonspb buttons to the content_toolbar. The mixer section now does also comply with the ESC/Motor Features styling. @mikeller having addressed stopping of motors properly when changing settings please take a look again. |
55138e1
to
fcb190d
Compare
fcb190d
to
282a458
Compare
Tools are disabled after changing settings too now. |
282a458
to
135f70c
Compare
135f70c
to
4d64cda
Compare
Kudos, SonarCloud Quality Gate passed! 13 Bugs No Coverage information |
@mikeller can we merge this? It seems finished to me. Some change is pending? |
@McGiverGim: I'll review. This has had so many repetitive changes pushed into it that I stopped looking at it. |
Congratulations, @haslinghuis very good job! :) |
Fixes: #2304
Summary:
Suggestions:
MOTOR_STOP
alignment withESC_SENSOR
and align padding on different places @TheIsotopesThanks @asizon @bizmar @chmelevskij @limonspb @McGiverGim @mikeller @TheIsotopes @ZZYZX