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

Add LED strip sliders #3531

Merged
merged 4 commits into from Aug 18, 2023
Merged

Add LED strip sliders #3531

merged 4 commits into from Aug 18, 2023

Conversation

ASDosjani
Copy link
Contributor

This PR add sliders to LED Strip tab for easier and quicker configuration with help icons. Brightness slider is always visible, Rainbow related slider only when the option is turned on.
From FC side here is the PR: betaflight/betaflight#12995

image

@github-actions

This comment has been minimized.

src/js/msp/MSPHelper.js Outdated Show resolved Hide resolved
src/tabs/led_strip.html Show resolved Hide resolved
src/tabs/led_strip.html Show resolved Hide resolved
Copy link
Member

@haslinghuis haslinghuis left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Jul 29, 2023

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 -> FAIL
  • approver count at least three -> FAIL

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Approved. I would like more consistency in the descriptions of the messages file, but it's ok.

For the future: the description must show the "context" to the translator. For example, for "Brightness", now you have"Brightness slider label" is better something like "Element of the LED Strip" or "Brightness of the LED Strip". With the context (LED Strip) the translator can give a more precise translation, when there are different translations on his language. The translator only sees the "message" and must look for the best translation.

@github-actions

This comment has been minimized.

@sonarcloud
Copy link

sonarcloud bot commented Aug 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

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!

@sugaarK sugaarK merged commit 7bca5f9 into betaflight:master Aug 18, 2023
9 checks passed
@nerdCopter
Copy link
Member

nerdCopter commented Aug 18, 2023

if merging this broke usage of master 4.5, then does it also break all old firmware??

is semver checking not applied here?

haslinghuis added a commit that referenced this pull request Aug 18, 2023
asizon pushed a commit that referenced this pull request Aug 18, 2023
Revert "Add LED strip sliders (#3531)"

This reverts commit 7bca5f9.
@nerdCopter

This comment was marked as outdated.

@nerdCopter
Copy link
Member

I think this would be the semver checks, but this may be wrong or incomplete or more than required.
I would like @haslinghuis to verify.
Diff:

diff --git a/src/js/msp/MSPHelper.js b/src/js/msp/MSPHelper.js
index a1ab13b1..45218316 100644
--- a/src/js/msp/MSPHelper.js
+++ b/src/js/msp/MSPHelper.js
@@ -1298,13 +1298,18 @@ MspHelper.prototype.process_data = function(dataHandler) {
                 console.log('Led strip mode colors saved');
                 break;
             case MSPCodes.MSP_LED_STRIP_CONFIG_VALUES:
-                FC.LED_CONFIG_VALUES = {
-                    brightness: data.readU8(),
-                    rainbow_delta: data.readU8(),
-                    rainbow_freq: data.readU16(),
-                };
+                if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46)) {
+                    FC.LED_CONFIG_VALUES = {
+                        brightness: data.readU8(),
+                        rainbow_delta: data.readU8(),
+                        rainbow_freq: data.readU16(),
+                    };
+                }
                 break;
             case MSPCodes.MSP_SET_LED_STRIP_CONFIG_VALUES:
+                if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46)) {
+                    //future?
+                }
                 break;
             case MSPCodes.MSP_DATAFLASH_SUMMARY:
                 if (data.byteLength >= 13) {
@@ -2640,11 +2645,13 @@ MspHelper.prototype.sendLedStripModeColors = function(onCompleteCallback) {
 };
 
 MspHelper.prototype.sendLedStripConfigValues = function(onCompleteCallback) {
-    const buffer = [];
-    buffer.push8(FC.LED_CONFIG_VALUES.brightness);
-    buffer.push8(FC.LED_CONFIG_VALUES.rainbow_delta);
-    buffer.push16(FC.LED_CONFIG_VALUES.rainbow_freq);
-    MSP.send_message(MSPCodes.MSP_SET_LED_STRIP_CONFIG_VALUES, buffer, false, onCompleteCallback);
+    if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46)) {
+        const buffer = [];
+        buffer.push8(FC.LED_CONFIG_VALUES.brightness);
+        buffer.push8(FC.LED_CONFIG_VALUES.rainbow_delta);
+        buffer.push16(FC.LED_CONFIG_VALUES.rainbow_freq);
+        MSP.send_message(MSPCodes.MSP_SET_LED_STRIP_CONFIG_VALUES, buffer, false, onCompleteCallback);
+    }
 };
 
 MspHelper.prototype.serialPortFunctionMaskToFunctions = function(functionMask) {
diff --git a/src/js/tabs/led_strip.js b/src/js/tabs/led_strip.js
index 8ab1c18b..48e69928 100644
--- a/src/js/tabs/led_strip.js
+++ b/src/js/tabs/led_strip.js
@@ -636,6 +636,11 @@ led_strip.initialize = function (callback, scrollPosition) {
             $('div.rainbowFreqSlider label').first().text(val);
         });
 
+        if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_46)) {
+            $('div.rainbowFreqSlider input').show();
+        } else {
+            $('div.rainbowFreqSlider input').hide();
+        }
 
         $('a.save').on('click', function () {
             mspHelper.sendLedStripConfig(send_led_strip_colors);

@haslinghuis
Copy link
Member

image

is not needed as handled in

image

If this code is called often we might use the semver on the consumer of the MSP message instead.

@ASDosjani ASDosjani mentioned this pull request Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

6 participants