-
-
Notifications
You must be signed in to change notification settings - Fork 938
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
Expose Cloud Build Options and hide tabs and features #3332
Conversation
This comment has been minimized.
This comment has been minimized.
AUTOMERGE: (FAIL)
|
339367e
to
2994a79
Compare
This comment has been minimized.
This comment has been minimized.
7d00373
to
da9bf38
Compare
This comment has been minimized.
This comment has been minimized.
da9bf38
to
d0fb8ea
Compare
This comment has been minimized.
This comment has been minimized.
728293f
to
6f19015
Compare
This comment has been minimized.
This comment has been minimized.
src/js/BuildApi.js
Outdated
requestBuildOptions(key, onSuccess, onFailure) { | ||
const url = `${this._url}/api/builds/${key}/json`; | ||
$.get(url, function (data) { | ||
onSuccess(data); | ||
}).fail(xhr => { | ||
if (onFailure !== undefined) { | ||
onFailure(); | ||
} | ||
}); | ||
} |
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.
Can we promisify this instead then just use try/catch
or then/catch
? There are now n + 1 callback patterns in this codebase for callbacks 😆
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 are getting the features from the cloud build information? This is not the way. What if the build is very old, we will store ALL the builds in the time? What if the build is a local custom build? If we want to get this information, I think we need to create MSP commands in the firmware, that can tell us what features/other UI elements are enabled or not.
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 it's very old and not stored, just display defaults.
- personally don't like the idea of more MSP. (more data storage). maybe with future MSP2, there is cli
get
to determine existence or not? unsure of a clear solution, ATM.
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.
@chmelevskij was just following same pattern in the api.
@McGiverGim there is a msp call to retrieve the buildkey. With the buildkey we request data from the cloud api filling some MSP variables. We might be able to store the filter in the cloud api. We only can use this from semver 1.45.
Local builds will have no buildkey (need to check).
I'd like to lock some configuration fields with cloud parameters as to hide items that are not included in the cloud build but the user still was able to select. Thoughts?
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 I say, in my opinion, the configurator must work with customs builds too, but the change to cloud build is a very big step and the Configurator had left outdated showing features that does not exist.
Maybe with the new MSP call with CLI we can ask for features to the CLI, if it shows an error then does not exist.
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 think there's two different issues here.
The first is using the build key, the user can default their build options to that contained for that key. Key not being user specific but build specific. Without key (custom or key not found) the build options revert to default. The dependency on the api is not a problem here because, well you need it to get the build anyway!
The second is what actual features in configurator are enabled. Eg don't show GPS tab if GPS not built in. That should come from the firmware. Using the cli suggestion is good.
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.
@blckmn sorted now:
Buildkey availability determines behavior.
The tab issue is also resolved.
locales/en/messages.json
Outdated
"featureSONAR": { | ||
"featureRANGEFINDER": { | ||
"message": "Sonar" | ||
}, |
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 for the user this is more a "Sonar", maybe is better to rename it in the firmware and not here?
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.
Good call, will change RANGEFINDER to SONAR in firmware but RANGEFINDER is just one possible SONAR device?
src/js/Features.js
Outdated
self._features = features; | ||
|
||
if (semver.gte(config.apiVersion, API_VERSION_1_45) && FC.CONFIG.buildKey) { | ||
const filter = ["GPS", "DASHBOARD", "LED_STRIP", "OSD", "RANGEFINDER", "SERIAL_RX", "SERVOS", "SPI_RX", "TELEMETRY", "TRANSPONDER"]; |
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 think we are having too much hardcoded elements. This will require to maintain in coherence firmware/configurator. I understand to do this for features, that is something that we are trying to remove so it will not grow, but I don't know if we are planning to do the same for other UI elements.
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.
Yes this is hack, becomes the 'other' group contains also non-features. While features are very stable over time this PR is just enabling support for cloud parameters AND applying this to features as a demonstration.
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.
Have removed the hack and added a property to the feature instead.
Also made sure it works with non cloud builds
src/js/BuildApi.js
Outdated
requestBuildOptions(key, onSuccess, onFailure) { | ||
const url = `${this._url}/api/builds/${key}/json`; | ||
$.get(url, function (data) { | ||
onSuccess(data); | ||
}).fail(xhr => { | ||
if (onFailure !== undefined) { | ||
onFailure(); | ||
} | ||
}); | ||
} |
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 are getting the features from the cloud build information? This is not the way. What if the build is very old, we will store ALL the builds in the time? What if the build is a local custom build? If we want to get this information, I think we need to create MSP commands in the firmware, that can tell us what features/other UI elements are enabled or not.
IGNORE THIS COMMENT :)
This works:
But this does not:
|
6f19015
to
8462e7a
Compare
This comment has been minimized.
This comment has been minimized.
8462e7a
to
327a0b1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The |
This comment was marked as outdated.
This comment was marked as outdated.
Thanks, updated to semver 1.46. |
a75ab7f
to
07074e9
Compare
This comment has been minimized.
This comment has been minimized.
@nerdCopter please check again. 🤔 We might decouple features toggling tabs (like GPS) to being enabled by default (if GPS option is included in build) |
i'm seeing same behavior as last screenshot for 4.4.0. However, i'm not sure what to expect. |
@nerdCopter betaflight/betaflight#12308 was the last working for boards having same issue (all leds are solid indicating hard fault) |
unfortunately #12308 not working for TMOTORF7 :( i'll pan through some older SHA's |
#12388 needs a redesign now we are removing custom defaults. |
This comment was marked as outdated.
This comment was marked as outdated.
sorry @haslinghuis , i'm hiding last comment as it checks for 1.46 now. unsure how to further test without a live 1.46. |
You could test with commit |
|
It works with |
This comment was marked as outdated.
This comment was marked as outdated.
07074e9
to
255e766
Compare
Add cloud build options to auto-detect
255e766
to
0a54e25
Compare
Kudos, SonarCloud Quality Gate passed!
|
@nerdCopter fixed the tab issue (inverting check and make available for 1.45) |
Do you want to test this code? Here you have an automated build: |
|
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.
💪🚀
Please test with firmware betaflight/betaflight#12388 as this would address:
🤔 Future improvements:
As example a very simple build for a LOS build:
we see:
As for a more sophisticated build like:
we see:
Originally we see features which are not available in firmware: