Add support for IMU3+ #4490
Add support for IMU3+ #4490
Conversation
WalkthroughThe changes add support for multiple gyros with individual enable and alignment controls, update data processing for firmware ≥ 1.47 to handle arrays and enable masks, and dynamically generate UI controls for each gyro. Localization entries for sensor and VTX configuration are also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (configuration.js)
participant MSPHelper
participant FC State
User->>UI (configuration.js): Open sensor configuration tab
UI (configuration.js)->>MSPHelper: Request sensor alignment data
MSPHelper->>FC State: Parse sensor alignment arrays (if API ≥ 1.47)
FC State-->>MSPHelper: Provide alignment arrays and enable mask
MSPHelper-->>UI (configuration.js): Return alignment data
UI (configuration.js)->>UI (configuration.js): Dynamically render per-gyro controls
User->>UI (configuration.js): Change alignment/enable state for one or more gyros
UI (configuration.js)->>MSPHelper: Send updated alignment arrays and enable mask
MSPHelper->>FC State: Update internal alignment state
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/js/tabs/configuration.js (1)
874-874:⚠️ Potential issueFix potential undefined variable reference.
The variable
orientation_gyro_to_use_eis referenced here but is only defined in the legacy code path (line 569) for older firmware versions. This will cause a reference error when using firmware >= 1.47.- FC.SENSOR_ALIGNMENT.gyro_to_use = parseInt(orientation_gyro_to_use_e.val()); + if (semver.lt(FC.CONFIG.apiVersion, API_VERSION_1_47)) { + FC.SENSOR_ALIGNMENT.gyro_to_use = parseInt($("select.gyro_to_use").val()); + }
🧹 Nitpick comments (4)
locales/en/messages.json (1)
1540-1542: Consider clarifying the alignment label.The message is currently
"Sensor Alignment". For clarity and consistency with the “IMU Sensor ” prefix, you might want to rename this to"IMU Sensor Alignment"or ensure the surrounding UI makes it unambiguous.src/js/tabs/configuration.js (3)
213-223: Consider extracting alignments array as a constant.The alignments array could be defined as a module-level constant for better reusability and organization.
Move the alignments array outside the function:
+const SENSOR_ALIGNMENTS = [ + "CW 0°", + "CW 90°", + "CW 180°", + "CW 270°", + "CW 0° flip", + "CW 90° flip", + "CW 180° flip", + "CW 270° flip", + "Custom", +]; configuration.initialize = function (callback) { // ... existing code ... function process_html() { - const alignments = [ - "CW 0°", - "CW 90°", - "CW 180°", - "CW 270°", - "CW 0° flip", - "CW 90° flip", - "CW 180° flip", - "CW 270° flip", - "Custom", - ];Then update all references from
alignmentstoSENSOR_ALIGNMENTS.
264-266: Clean up commented test code.The commented test code should be removed to improve code clarity.
- // Check if this gyro is detected (uncomment for production) detected_gyros[i] = (FC.SENSOR_ALIGNMENT.gyro_detection_flags & GYRO_DETECTION_FLAGS[`DETECTED_GYRO_${i + 1}`]) != 0; - // detected_gyros[i] = true; // For testing
448-448: Remove commented CSS properties.The commented background-color properties should be removed from the CSS string.
padding: 12px; margin-bottom: 15px; - // background-color: #f8f8f8; box-shadow: 0 1px 3px rgba(0,0,0,0.05);And:
padding: 10px; border-radius: 3px; - // background-color: #f1f1f1; border: 1px solid #e5e5e5;Also applies to: 492-492
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
locales/en/messages.json(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(2 hunks)src/js/tabs/configuration.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/js/tabs/configuration.js (4)
src/js/fc.js (1)
FC(130-993)src/js/data_storage.js (4)
API_VERSION_1_47(5-5)API_VERSION_1_47(5-5)API_VERSION_1_46(4-4)API_VERSION_1_46(4-4)src/js/sensor_helpers.js (1)
have_sensor(4-22)src/js/sensor_types.js (2)
sensorTypes(6-126)sensorTypes(7-104)
src/js/msp/MSPHelper.js (2)
src/js/fc.js (2)
FC(130-993)i(263-263)src/js/data_storage.js (2)
API_VERSION_1_47(5-5)API_VERSION_1_47(5-5)
🔇 Additional comments (5)
src/js/fc.js (1)
481-484:✅ Verification successful
LGTM! Multi-gyro alignment support correctly implemented.
The conversion from scalar values to arrays properly enables support for multiple gyros (up to 8) as intended for IMU3+ support. The implementation is consistent with the architectural changes described in the AI summary.
Please verify that all code dependent on these properties has been updated to handle arrays instead of scalars:
🏁 Script executed:
#!/bin/bash # Description: Verify that all references to gyro alignment properties handle the new array format # Expected: All usages should be compatible with array format for API 1.47+ echo "Searching for usages of gyro alignment properties..." rg -A 3 -B 1 "gyro_align_(roll|pitch|yaw)" --type js rg -A 3 -B 1 "\.gyro_align\b" --type js echo "Checking for potential scalar assignments or mathematical operations..." rg -A 2 -B 2 "(gyro_align_roll|gyro_align_pitch|gyro_align_yaw)\s*[+\-*/=]" --type jsLength of output: 6767
Confirmed multi-gyro alignment arrays consistently handled
All references to
gyro_align,gyro_align_roll,gyro_align_pitch, andgyro_align_yawnow use array formats throughout the codebase:
- In src/js/fc.js, the default values are initialized as
[].- In src/js/msp/MSPHelper.js, alignment data is read into and pushed from these arrays in loops over 8 gyros.
- In src/js/tabs/configuration.js, arrays are initialized when missing and each gyro’s roll/pitch/yaw values are read from or written to the correct array index.
- No remaining scalar assignments or arithmetic operations are performed directly on these properties.
LGTM—no further changes needed.
locales/en/messages.json (2)
1534-1536: Verify usage of trailing space in IMU Sensor title.The message
"IMU Sensor "includes a trailing space, presumably to concatenate with a sensor index (e.g., “IMU Sensor 1”). Please confirm that the UI code appends the index correctly and that the extra space is intentional and not causing layout issues.
1537-1539: Ensure translations for 'Enable Sensor' are added across all locales.This new key
configurationSensorEnablemust be present in every supported language file so users see the “Enable Sensor” label consistently. Verify that you’ve propagated this entry to alllocales/*/messages.json.src/js/msp/MSPHelper.js (2)
641-669: LGTM! Correct implementation of multi-gyro sensor alignment reading.The code properly handles the new multi-gyro format for API version 1.47+ while maintaining backward compatibility. The scaling factor is correctly applied when reading the 16-bit alignment values.
2095-2126: LGTM! Consistent implementation of multi-gyro sensor alignment writing.The writing logic correctly mirrors the reading logic, applying the appropriate scaling factor (*10) and maintaining the same data structure. Backward compatibility is properly preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/js/tabs/configuration.js (1)
84-914: 🛠️ Refactor suggestionConsider refactoring the process_html function for better maintainability.
The
process_htmlfunction is extremely large (over 800 lines) and handles multiple concerns. This violates the Single Responsibility Principle and makes the code hard to maintain and test.Consider breaking this function into smaller, focused functions:
function process_html() { self.analyticsChanges = {}; - // All the existing logic... + + setupFeatures(); + setupBeepers(); + setupGyroConfiguration(); + setupSensorConfiguration(); + setupMagnetometerConfiguration(); + setupEventHandlers(); + setupStatusPolling(); } + +function setupFeatures() { + // Feature configuration logic +} + +function setupBeepers() { + // Beeper configuration logic +} + +function setupGyroConfiguration() { + // Gyro configuration logic (lines 225-632) +} + +function setupSensorConfiguration() { + // Other sensor configuration logic +} + +function setupMagnetometerConfiguration() { + // Magnetometer configuration logic +} + +function setupEventHandlers() { + // Event handler setup +} + +function setupStatusPolling() { + // Status polling setup +}This would improve:
- Testability: Each function can be tested independently
- Readability: Each function has a clear, single purpose
- Maintainability: Changes to one concern don't affect others
- Reusability: Functions can be reused in other contexts
🧹 Nitpick comments (2)
src/js/tabs/configuration.js (2)
225-239: Initialize MAX_GYROS as a module-level constant.The
MAX_GYROSconstant is currently defined inside the function scope. Consider moving it to module level for better reusability and consistency.+const MAX_GYROS = 8; + const configuration = { analyticsChanges: {}, };Then remove the local definition:
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47)) { - // Define constants - const MAX_GYROS = 8;
262-431: Consider extracting gyro UI generation into separate functions.The gyro UI generation logic is quite complex and deeply nested. Breaking it into smaller functions would improve readability and testability.
+ function createGyroBox(gyroIndex, detected_gyros) { + // Move gyro box creation logic here + } + + function setupGyroEventHandlers(gyroIndex, elements) { + // Move event handler setup here + } // For each possible gyro for (let i = 0; i < MAX_GYROS; i++) { detected_gyros[i] = (FC.SENSOR_ALIGNMENT.gyro_detection_flags & GYRO_DETECTION_FLAGS[`DETECTED_GYRO_${i + 1}`]) != 0; if (detected_gyros[i]) { - // Large block of UI creation code + const gyroElements = createGyroBox(i, detected_gyros); + setupGyroEventHandlers(i, gyroElements); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
locales/en/messages.json(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(2 hunks)src/js/tabs/configuration.js(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- locales/en/messages.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/fc.js
- src/js/msp/MSPHelper.js
🔇 Additional comments (2)
src/js/tabs/configuration.js (2)
19-29: LGTM! Good centralization of sensor alignment options.The
SENSOR_ALIGNMENTSconstant provides a clean, centralized definition of all sensor alignment options, improving maintainability and consistency across the codebase.
817-831: 🛠️ Refactor suggestionVerify array initialization and improve error handling.
The array initialization and value gathering logic needs validation to prevent potential runtime errors.
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47)) { + const MAX_GYROS = 8; // Use the module constant when available + // Initialize arrays if they don't exist if (!Array.isArray(FC.SENSOR_ALIGNMENT.gyro_align_roll)) { FC.SENSOR_ALIGNMENT.gyro_align_roll = new Array(MAX_GYROS).fill(0); FC.SENSOR_ALIGNMENT.gyro_align_pitch = new Array(MAX_GYROS).fill(0); FC.SENSOR_ALIGNMENT.gyro_align_yaw = new Array(MAX_GYROS).fill(0); } // Gather custom alignment values for all detected gyros for (let i = 0; i < MAX_GYROS; i++) { + // Ensure arrays have sufficient length + if (i >= FC.SENSOR_ALIGNMENT.gyro_align_roll.length) { + FC.SENSOR_ALIGNMENT.gyro_align_roll.length = MAX_GYROS; + FC.SENSOR_ALIGNMENT.gyro_align_pitch.length = MAX_GYROS; + FC.SENSOR_ALIGNMENT.gyro_align_yaw.length = MAX_GYROS; + } + // Get the values from the inputs and store in the arrays - FC.SENSOR_ALIGNMENT.gyro_align_roll[i] = parseInt($(`.gyro_${i + 1}_align_roll`).val()) || 0; - FC.SENSOR_ALIGNMENT.gyro_align_pitch[i] = parseInt($(`.gyro_${i + 1}_align_pitch`).val()) || 0; - FC.SENSOR_ALIGNMENT.gyro_align_yaw[i] = parseInt($(`.gyro_${i + 1}_align_yaw`).val()) || 0; + const rollInput = $(`.gyro_${i + 1}_align_roll`); + const pitchInput = $(`.gyro_${i + 1}_align_pitch`); + const yawInput = $(`.gyro_${i + 1}_align_yaw`); + + FC.SENSOR_ALIGNMENT.gyro_align_roll[i] = rollInput.length ? parseInt(rollInput.val()) || 0 : 0; + FC.SENSOR_ALIGNMENT.gyro_align_pitch[i] = pitchInput.length ? parseInt(pitchInput.val()) || 0 : 0; + FC.SENSOR_ALIGNMENT.gyro_align_yaw[i] = yawInput.length ? parseInt(yawInput.val()) || 0 : 0; }Likely an incorrect or invalid review comment.
haslinghuis
force-pushed
the
imu3
branch
2 times, most recently
from
c796079 to
4e2da1f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/js/tabs/configuration.js (1)
881-881:⚠️ Potential issueFix undefined variable reference in save logic.
The code references
orientation_gyro_to_use_ewhich is only defined in the legacy code path (firmware < 1.47), but this line executes for all firmware versions, potentially causing a runtime error.Apply this fix to handle the version-specific logic:
- FC.SENSOR_ALIGNMENT.gyro_to_use = parseInt(orientation_gyro_to_use_e.val()); + if (semver.lt(FC.CONFIG.apiVersion, API_VERSION_1_47)) { + FC.SENSOR_ALIGNMENT.gyro_to_use = parseInt(orientation_gyro_to_use_e.val()); + }
♻️ Duplicate comments (1)
src/js/tabs/configuration.js (1)
434-557: CSS injection issue remains unaddressed.The large CSS block injection flagged in previous reviews has not been resolved. This still impacts performance (injection on every tab load), maintainability (CSS embedded in JavaScript), and security (string concatenation risks).
🧹 Nitpick comments (1)
src/js/tabs/configuration.js (1)
228-228: Move MAX_GYROS to global scope for consistency.The
MAX_GYROSconstant is defined locally but might be needed in other parts of the codebase. Consider moving it to the global scope alongsideSENSOR_ALIGNMENTS.const SENSOR_ALIGNMENTS = [ "CW 0°", "CW 90°", "CW 180°", "CW 270°", "CW 0° flip", "CW 90° flip", "CW 180° flip", "CW 270° flip", "Custom", ]; + +const MAX_GYROS = 8;Then remove the local definition and use the global constant.
🛑 Comments failed to post (1)
src/js/tabs/configuration.js (1)
226-632: 🛠️ Refactor suggestion
Refactor large multi-gyro handling function into smaller, focused functions.
This 400+ line block handles multiple responsibilities: UI generation, event binding, styling, and state management. This violates the single responsibility principle and makes the code difficult to maintain.
Consider breaking this into smaller functions:
+ function initializeMultiGyroSupport() { + const MAX_GYROS = 8; + const detected_gyros = detectGyros(MAX_GYROS); + + if (detected_gyros.some(detected => detected)) { + createMultiGyroUI(detected_gyros, MAX_GYROS); + bindMultiGyroEventHandlers(detected_gyros, MAX_GYROS); + } + } + + function detectGyros(maxGyros) { + // Move gyro detection logic here + } + + function createMultiGyroUI(detectedGyros, maxGyros) { + // Move UI creation logic here + } + + function bindMultiGyroEventHandlers(detectedGyros, maxGyros) { + // Move event handler binding here + } - // Multi gyro handling for newer firmware - if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47)) { - // [400+ lines of mixed logic] - } + if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_47)) { + initializeMultiGyroSupport(); + }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/js/tabs/configuration.js lines 226 to 632, the large multi-gyro handling block mixes UI creation, event binding, styling, and state management, making it hard to maintain. Refactor by extracting smaller functions for each responsibility: one for generating gyro UI elements, one for binding event handlers, one for applying styles, and one for managing gyro state initialization and updates. Replace the large block with calls to these focused functions to improve readability and maintainability.
haslinghuis
force-pushed
the
imu3
branch
4 times, most recently
from
3d1994e to
799c30b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/js/tabs/configuration.js (1)
436-436:⚠️ Potential issueRemove debug console.log statement.
Debug logging should not be present in production code.
-console.log("At least one gyro must remain enabled");
🧹 Nitpick comments (2)
src/js/msp/MSPHelper.js (1)
651-653: Consider documenting the maximum gyro count.The hardcoded loop limit of 8 gyros appears to be a firmware constraint, but this should be documented or made configurable for maintainability.
+ // Maximum supported gyros per firmware specification for (let i = 0; i < 8; i++) { FC.SENSOR_ALIGNMENT.gyro_align[i] = data.readU8(); }locales/en/messages.json (1)
1534-1536: Remove unintended trailing whitespace in title message
The value for"configurationSensorGyroTitle"ends with a space ("IMU Sensor "). If this space is not intentionally used for concatenation in the UI, please trim it to"IMU Sensor"to avoid subtle formatting issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(1 hunks)src/css/tabs/configuration.less(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(2 hunks)src/js/tabs/configuration.js(3 hunks)
🔇 Additional comments (8)
src/js/fc.js (1)
481-484: LGTM! Proper data structure change for multi-gyro support.The change from scalar values to arrays for gyro alignment properties correctly supports the new multi-gyro configuration feature. This allows storing alignment data for multiple gyros (up to 8 as mentioned in the AI summary) rather than just single values.
src/css/tabs/configuration.less (1)
57-141: Excellent CSS implementation for the multi-gyro UI.The new CSS styles are well-structured and follow good practices:
✅ Layout: Proper use of flexbox for responsive layout
✅ Visual hierarchy: Clear typography and spacing with.gyro_title,.gyro_rowstructure
✅ Interactivity: Appropriate hover and focus states for better UX
✅ Consistency: Uniform color scheme and spacing throughout
✅ Organization: Logical class naming and nestingThe styles will provide a clean, intuitive interface for the new multi-gyro configuration feature.
src/js/msp/MSPHelper.js (3)
642-669: LGTM! Well-structured multi-gyro support with proper backwards compatibility.The implementation correctly handles the transition from single gyro to multi-gyro support:
- Properly replaces
gyro_to_usewithgyro_enable_maskfor API 1.47+- Initializes arrays for up to 8 gyros with individual alignment parameters
- Maintains consistent data scaling (division by 10 for angle values)
- Preserves backwards compatibility for older firmware versions
2101-2125: LGTM! Consistent write implementation matching the read logic.The write implementation correctly mirrors the read logic:
- Uses the same API version check
- Applies proper scaling (multiplication by 10) that matches the read scaling
- Maintains the same loop structure for 8 gyros
- Preserves backwards compatibility
The data flow is consistent between read and write operations.
2108-2113:✅ Verification successful
Verify data scaling consistency.
The scaling factor appears correct (multiply by 10 for transmission, divide by 10 on reception), but please ensure this matches the firmware expectations for angle precision.
🏁 Script executed:
#!/bin/bash # Verify the scaling factor usage is consistent throughout the codebase echo "Searching for gyro alignment scaling patterns..." rg -A 3 -B 3 "gyro_align.*\* 10|gyro_align.*/ 10" --type jsLength of output: 1407
Scaling consistency verified.
I’ve confirmed that the code applies
* 10when writing (push16) and/ 10when reading (read16) for all gyro alignment values inMSPHelper.js, ensuring consistent scaling. No discrepancies were found.locales/en/messages.json (1)
1534-1545: Add localization keys for multi-gyro UI controls and warnings
The new entries (configurationSensorGyroTitle,configurationSensorEnable,configurationSensorAlignment,configurationGyroRequiredWarning) correctly follow the existing JSON structure and naming conventions, providing clear labels and warning messages for the multi-gyro configuration UI.src/js/tabs/configuration.js (2)
20-33: Good constant definitions for multi-gyro support.The
SENSOR_ALIGNMENTSarray andMAX_GYROSconstant are well-defined and appropriately placed at the module level.
228-449: Well-implemented multi-gyro support with proper safety checks.The dynamic UI generation for multiple gyros is comprehensive and includes:
- Proper detection of available gyros
- Safety check preventing all gyros from being disabled
- Dynamic alignment controls for each gyro
- Appropriate event handlers
The implementation correctly handles the transition to API v1.47+ multi-gyro support.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/js/tabs/configuration.js (2)
270-270:⚠️ Potential issueRemove debug console.log statements.
Debug logging should not be present in production code. The user-facing warning is already properly handled with
gui_logon line 437.- console.log(`Creating UI for gyro ${i + 1}`);- console.log("At least one gyro must remain enabled");Also applies to: 436-436
706-711:⚠️ Potential issueRemove commented code blocks for clarity.
These commented sections create confusion about the intended behavior. As noted in past reviews, the current implementation correctly handles all gyros by defaulting to 0 for non-detected ones.
- // Initialize arrays if they don't exist - // if (!Array.isArray(FC.SENSOR_ALIGNMENT.gyro_align_roll)) { - // FC.SENSOR_ALIGNMENT.gyro_align = new Array(MAX_GYROS).fill(1); - // FC.SENSOR_ALIGNMENT.gyro_align_roll = new Array(MAX_GYROS).fill(0); - // FC.SENSOR_ALIGNMENT.gyro_align_pitch = new Array(MAX_GYROS).fill(0); - // FC.SENSOR_ALIGNMENT.gyro_align_yaw = new Array(MAX_GYROS).fill(0); - // } -// Gather custom alignment values for all gyros for (let i = 0; i < MAX_GYROS; i++) { - // Get the values from the inputs and store in the arrays FC.SENSOR_ALIGNMENT.gyro_align_roll[i] = parseInt($(`.gyro_${i + 1}_align_roll`).val()) || 0; FC.SENSOR_ALIGNMENT.gyro_align_pitch[i] = parseInt($(`.gyro_${i + 1}_align_pitch`).val()) || 0; FC.SENSOR_ALIGNMENT.gyro_align_yaw[i] = parseInt($(`.gyro_${i + 1}_align_yaw`).val()) || 0; }Also applies to: 729-735
🧹 Nitpick comments (3)
locales/en/messages.json (1)
1534-1545: New i18n entries are correctly structured
The added keys align with the existing pattern and maintain valid JSON.
Suggest a quick nitpick: remove the trailing space in the"IMU Sensor "message (or switch to a placeholder like"IMU Sensor {index}") to avoid accidental double-spaces at render time, and verify that theconfigurationGyroRequiredWarningkey name matches its usage in the code.src/js/msp/MSPHelper.js (1)
651-653: Consider using a constant for the maximum gyro count.The magic number
8appears multiple times representing the maximum number of supported gyros. This should be defined as a constant to improve maintainability and readability.Consider adding a constant at the top of the file:
+const MAX_GYROS = 8; + function MspHelper() {Then replace the magic numbers:
- for (let i = 0; i < 8; i++) { + for (let i = 0; i < MAX_GYROS; i++) { FC.SENSOR_ALIGNMENT.gyro_align[i] = data.readU8(); } - for (let i = 0; i < 8; i++) { + for (let i = 0; i < MAX_GYROS; i++) { FC.SENSOR_ALIGNMENT.gyro_align_roll[i] = data.read16() / 10;Also applies to: 655-659, 2104-2106, 2108-2113
src/js/tabs/configuration.js (1)
228-451: Well-architected multi-gyro support implementation.The dynamic UI generation approach scales elegantly to support up to 8 gyros while maintaining backward compatibility. Consider documenting the gyro detection flags and enable mask behavior for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(1 hunks)src/css/tabs/configuration.less(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(2 hunks)src/js/tabs/configuration.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
locales/en/messages.json (1)
undefined
<retrieved_learning>
Learnt from: haslinghuis
PR: #4490
File: locales/en/messages.json:1534-1545
Timestamp: 2025-05-26T11:37:09.741Z
Learning: In the Betaflight Configurator repository, only English messages should be added directly to locales/en/messages.json; other language translations are handled via Crowdin automation and should not be manually updated in PRs.
</retrieved_learning>
🔇 Additional comments (7)
src/js/fc.js (1)
481-484:✅ Verification successful
LGTM! Array initialization supports multi-gyro functionality.
The change from scalar values to arrays correctly enables support for multiple gyros with individual alignment settings. This aligns with the PR's objective of adding IMU3+ support.
However, consider whether these arrays should be initialized with default values based on the expected number of gyros rather than empty arrays, to ensure consistent behavior across the application.
🏁 Script executed:
#!/bin/bash # Verify that all references to these gyro alignment fields handle the new array format echo "Searching for usage of gyro alignment fields to ensure compatibility with array format..." rg -A 3 -B 3 "gyro_align" --type jsLength of output: 14024
Verify empty-array initialization for gyro alignment
After searching all references to
gyro_align*, I confirmed:
- In
src/js/msp/MSPHelper.js, loops read into and push fromgyro_align,gyro_align_roll,gyro_align_pitch, andgyro_align_yawfor indices 0–7.- In
src/js/tabs/configuration.js, each undefined entry is explicitly defaulted to 0 when rendering and collecting values.- No other code assumes a scalar value—every access uses array indexing with fallbacks.
Empty arrays won’t cause breaking behavior because undefined entries are handled. If you’d like to simplify the code and avoid those runtime checks, you could pre-fill each array to
MAX_GYROS(e.g.new Array(MAX_GYROS).fill(0)or.fill(1)forgyro_align) upon initialization. Otherwise, these changes correctly support multi-gyro functionality with no further action needed.src/css/tabs/configuration.less (1)
57-141: Excellent CSS implementation for the multi-gyro UI.The added styles provide a comprehensive and well-structured foundation for the gyro configuration interface:
✅ Strengths:
- Clean, modern layout using flexbox
- Proper spacing and visual hierarchy
- Interactive states (hover, focus) for better UX
- Consistent naming conventions
- Responsive design considerations
- Accessible styling with proper contrast and spacing
✅ Best Practices:
- Logical organization of nested selectors
- Appropriate use of margins and padding
- Clear visual feedback for user interactions
- Maintainable CSS structure
The styling effectively supports the dynamic multi-gyro configuration functionality mentioned in the PR objectives.
src/js/msp/MSPHelper.js (2)
642-669: Excellent implementation of multi-gyro support with proper backward compatibility.The implementation correctly handles the transition from the legacy 2-gyro system to the new multi-gyro architecture:
- ✅ Proper API version checks for backward compatibility
- ✅ Consistent data format conversion (÷10 on read, ×10 on write)
- ✅ Clean separation between new array-based and legacy individual field approaches
- ✅ Maintains all existing magnetometer alignment functionality
The code structure follows the established patterns in the codebase and properly handles the protocol changes introduced in API version 1.47.
Also applies to: 2101-2125
645-649:✅ Verification successful
Verify array initialization in FC.SENSOR_ALIGNMENT object.
The code initializes empty arrays for gyro alignment properties, but we should ensure these arrays are properly initialized in the FC object structure to prevent potential runtime errors.
🏁 Script executed:
#!/bin/bash # Check if SENSOR_ALIGNMENT arrays are properly initialized in fc.js echo "Searching for SENSOR_ALIGNMENT initialization in fc.js..." rg -A 10 -B 5 "SENSOR_ALIGNMENT.*=" --type js echo -e "\nChecking for gyro_align array initialization..." rg "gyro_align.*\[" --type jsLength of output: 33391
No action needed on array initialization
The
SENSOR_ALIGNMENTobject insrc/js/fc.jsalready definesgyro_align,gyro_align_roll,gyro_align_pitch, andgyro_align_yawas empty arrays. InMSPHelper.js, these same arrays are deliberately reinitialized to[]before populating them via the subsequent loops, ensuring there won’t be any undefined-index errors at runtime.src/js/tabs/configuration.js (3)
14-14: LGTM! Well-structured constants for multi-gyro support.The
gui_logimport and sensor alignment constants are properly defined for the new IMU3+ feature.Also applies to: 20-33
429-443: Excellent safety check implementation!The logic correctly prevents users from disabling all gyros, maintaining at least one active gyro at all times. The user feedback via
gui_logis appropriate.
576-602: Clean implementation of new sensor support.The range finder and optical flow sensor configuration is properly gated by API version checks and uses the centralized
sensorTypes()for consistency.
haslinghuis
force-pushed
the
imu3
branch
2 times, most recently
from
1f6f126 to
fa1506a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/js/msp/MSPHelper.js (1)
644-671: The multi-gyro sensor alignment reading looks good, with proper backward compatibility.The array initialization and data reading logic is correct. However, consider adding a comment explaining that the firmware always sends data for MAX_GYROS entries, even if fewer gyros are physically present.
Consider adding a comment above line 653 to clarify the data format:
+ // Firmware always sends alignment data for MAX_GYROS entries for (let i = 0; i < MAX_GYROS; i++) {src/js/tabs/configuration.js (2)
228-444: Consider refactoring the large multi-gyro initialization block.While the multi-gyro functionality is correctly implemented with proper detection logic, safety checks, and event handlers, this code block is quite large (200+ lines) and handles multiple responsibilities.
The implementation correctly:
- Detects available gyros using bitmask flags
- Creates dynamic UI for each detected gyro
- Implements safety checks to prevent disabling all gyros
- Handles custom alignment values properly
However, for better maintainability, consider extracting the UI creation logic into separate functions:
+ function createGyroAlignmentUI(gyroIndex, gyroContainer) { + // Move the gyro box creation logic here + } + + function setupGyroEventHandlers(gyroIndex, alignmentSelect, enableCheck, customInputs) { + // Move the event handler setup here + } + // For each possible gyro for (let i = 0; i < MAX_GYROS; i++) { if (detected_gyros[i]) { - // Large UI creation block... + createGyroAlignmentUI(i, gyroContainer); + setupGyroEventHandlers(i, alignmentSelect, enableCheck, customInputs); } }
371-371: Consider using a constant for custom alignment detection.The hardcoded value
9for detecting custom alignment should reference the position in theSENSOR_ALIGNMENTSarray for better maintainability.-const isCustom = parseInt(FC.SENSOR_ALIGNMENT.gyro_align[i]) === 9; +const isCustom = parseInt(FC.SENSOR_ALIGNMENT.gyro_align[i]) === SENSOR_ALIGNMENTS.length;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(3 hunks)src/css/tabs/configuration.less(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(3 hunks)src/js/tabs/configuration.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/fc.js
🧰 Additional context used
🧠 Learnings (1)
locales/en/messages.json (1)
undefined
<retrieved_learning>
Learnt from: haslinghuis
PR: #4490
File: locales/en/messages.json:1534-1545
Timestamp: 2025-05-26T11:37:09.741Z
Learning: In the Betaflight Configurator repository, only English messages should be added directly to locales/en/messages.json; other language translations are handled via Crowdin automation and should not be manually updated in PRs.
</retrieved_learning>
🔇 Additional comments (12)
src/css/tabs/configuration.less (1)
57-141: LGTM! Well-structured CSS for the multi-gyro configuration UI.The styling follows consistent naming conventions and uses flexbox appropriately for responsive layouts. The hover effects and input focus states provide good UX feedback.
src/js/msp/MSPHelper.js (2)
26-27: LGTM! Clear constant definition for maximum supported gyros.Good practice to define this as a module-level constant rather than using magic numbers.
2103-2127:✅ Verification successful
The multi-gyro sensor alignment writing implementation is correct and consistent.
The write logic properly mirrors the read logic, including the angle value scaling (*10 for write, /10 for read) and backward compatibility handling.
Let me verify that the angle scaling is consistent across the codebase:
🏁 Script executed:
#!/bin/bash # Check for other occurrences of gyro_align angle scaling to ensure consistency rg -A 2 -B 2 "gyro_align_(roll|pitch|yaw)" --type js | grep -E "([\*/])\s*10"Length of output: 722
Approve: Multi-gyro sensor alignment write logic
The implementation in
src/js/msp/MSPHelper.js(lines 2103–2127) correctly mirrors the read logic:
- Uses
* 10on write and/ 10on read for roll, pitch, and yaw angles.- Maintains backward compatibility for API versions below 1.47.
src/js/tabs/configuration.js (6)
14-14: LGTM!The
gui_logimport is appropriately added to support user messaging functionality used later in the gyro safety checks.
20-33: LGTM!The constants are well-defined and improve code maintainability. The
SENSOR_ALIGNMENTSarray centralizes alignment options, andMAX_GYROS = 8clearly defines the system limit.
425-432: LGTM!Excellent safety check implementation that prevents users from disabling all gyros, which would render the flight controller non-functional. The user feedback via
gui_logis also well-implemented.
445-516: LGTM!The backward compatibility handling for older firmware versions is well-structured and maintains the original functionality while cleanly separating the new multi-gyro logic.
697-728: LGTM!The save functionality correctly handles the transition from scalar values to arrays for multi-gyro support. The array initialization with proper defaults (fill(1) for alignment, fill(0) for custom values) and the specific selector usage on line 719 address previous review concerns.
733-737: LGTM!The conditional handling maintains compatibility with older firmware versions by preserving the original scalar value assignment logic.
locales/en/messages.json (3)
1534-1542: Add sensor gyro configuration keys
The new translation entries for enabling gyros, alignment labels, and the required-warning message are correctly added with clear English messages and appropriate descriptions. Naming is consistent with existing keys.
3155-3156: Add motors gyro select translation key
ThemotorsSensorGyroSelectentry leverages the existing short-gyro status key and includes a non-translatable description. This matches the pattern used elsewhere.
4751-4753: Add PID tuning iterm relax type gyro option key
ThepidTuningItermRelaxTypeOptionGyrokey is correctly introduced, reusing the short-gyro status translation. Description clarifies it shouldn’t be translated.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/js/tabs/configuration.js (1)
264-435: Consider breaking down this large function for better maintainability.The
createGyroAlignmentUIfunction is handling multiple responsibilities across 171 lines. Consider extracting helper functions to improve readability and maintainability:
- Extract UI element creation (lines 266-350)
- Extract event handler setup (lines 370-434)
- Extract custom alignment initialization (lines 329-337)
This would make the code more modular and easier to test.
Example refactoring approach:
function createGyroAlignmentUI(gyroIndex, container) { const gyroBox = createGyroBox(gyroIndex); const { alignmentSelect, enableCheck } = createGyroControls(gyroIndex); const customAlignSection = createCustomAlignmentSection(gyroIndex); initializeGyroValues(gyroIndex, alignmentSelect, enableCheck); setupGyroEventHandlers(gyroIndex, alignmentSelect, enableCheck, customAlignSection); gyroBox.append(combinedRow, customAlignSection); container.append(gyroBox); gyro_align_elements[gyroIndex] = alignmentSelect; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(3 hunks)src/css/tabs/configuration.less(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(3 hunks)src/js/tabs/configuration.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/css/tabs/configuration.less
🧰 Additional context used
🧠 Learnings (1)
locales/en/messages.json (1)
undefined
<retrieved_learning>
Learnt from: haslinghuis
PR: #4490
File: locales/en/messages.json:1534-1545
Timestamp: 2025-05-26T11:37:09.741Z
Learning: In the Betaflight Configurator repository, only English messages should be added directly to locales/en/messages.json; other language translations are handled via Crowdin automation and should not be manually updated in PRs.
</retrieved_learning>
🔇 Additional comments (10)
src/js/fc.js (1)
481-484:✅ Verification successful
Good structural change for multi-gyro support, but consider proper initialization.
The transition from scalar values to arrays correctly supports the new multi-gyro functionality. However, initializing these arrays as empty
[]might not provide appropriate default values for gyro alignment.Consider initializing these arrays with proper default values or at least document the expected array length:
- gyro_align: [], - gyro_align_roll: [], - gyro_align_pitch: [], - gyro_align_yaw: [], + gyro_align: Array(8).fill(0), // Support up to 8 gyros with default alignment + gyro_align_roll: Array(8).fill(0), + gyro_align_pitch: Array(8).fill(0), + gyro_align_yaw: Array(8).fill(0),Please verify that all code consuming these properties has been updated to handle arrays instead of scalar values:
🏁 Script executed:
#!/bin/bash # Description: Find all references to the changed gyro alignment properties to ensure they handle arrays properly # Expected: All usages should treat these as arrays, not scalar values echo "Searching for gyro_align property usage..." rg -A 3 -B 3 "gyro_align[^_]" --type js echo "Searching for gyro_align_roll property usage..." rg -A 3 -B 3 "gyro_align_roll" --type js echo "Searching for gyro_align_pitch property usage..." rg -A 3 -B 3 "gyro_align_pitch" --type js echo "Searching for gyro_align_yaw property usage..." rg -A 3 -B 3 "gyro_align_yaw" --type jsLength of output: 24609
Empty-array initialization is appropriate; no changes needed.
The
gyro_align*properties are correctly initialized as empty arrays inresetState(). Subsequent code paths populate these arrays with default values:
- In
src/js/tabs/configuration.js, for firmware ≥ 1.47, arrays are created withMAX_GYROSentries vianew Array(MAX_GYROS).fill(...).- In
src/js/msp/MSPHelper.js, arrays are reinitialized before reading sensor data into each index.Given these safeguards, you can safely leave the initializers as
[].src/js/msp/MSPHelper.js (3)
26-27: LGTM! Appropriate constant definition for multi-gyro support.The
MAX_GYROSconstant with value 8 provides a clear upper bound for gyro array iterations and aligns with the PR's objective of supporting multiple gyros.
643-671: Well-implemented multi-gyro alignment support with proper backward compatibility.The code correctly handles the transition from single gyro selection (
gyro_to_use) to a bitmask-based enable system (gyro_enable_mask) for API version 1.47+. The implementation properly:
- Initializes all required arrays before use
- Reads alignment data for all gyros with appropriate scaling (÷10 for decimal precision)
- Maintains backward compatibility for older firmware versions
- Preserves magnetometer alignment handling
2097-2128: Correctly implements sensor alignment writing for multi-gyro support.The writing logic properly mirrors the reading implementation:
- Uses
gyro_enable_maskfor API 1.47+ while maintaining backward compatibility- Correctly scales alignment angles (×10) to match the protocol's decimal precision
- Iterates through all gyros using the
MAX_GYROSconstant- Maintains consistent structure with the reading code
locales/en/messages.json (3)
1534-1543: Add new English i18n keys for multi-gyro support
The three new entries—configurationSensorGyroEnable,configurationSensorAlignment, andconfigurationGyroRequiredWarning—correctly introduce the UI labels and warning text needed for IMU3+ support. They’re scoped toenonly, consistent with Crowdin handling other translations. Also verify that the keysensorStatusGyroShortexists earlier in this file.
3155-3156: Refactor static “gyro” label to dynamic translation
Replacing the literal"gyro"with$t(sensorStatusGyroShort.message)inmotorsSensorGyroSelectensures consistent reuse of the existing short-status key. Including the"Don't translate!!!"description is spot on.
4752-4753: Use dynamic sensor status key for Iterm Relax Type option
Switching the"Gyro"dropdown option to$t(sensorStatusGyroShort.message)inpidTuningItermRelaxTypeOptionGyrokeeps the UI consistent with other dynamic sensor labels. TheDon't translate!!!descriptor is correct.src/js/tabs/configuration.js (3)
14-14: LGTM! Well-structured constants for multi-gyro support.The addition of
gui_logimport and the new constants (SENSOR_ALIGNMENTSandMAX_GYROS) are properly defined and will support the dynamic multi-gyro configuration.Also applies to: 20-33
236-243: Excellent safety measures for gyro enable mask initialization.The code properly handles the initialization of
gyro_enable_maskwith appropriate defaults and includes a crucial safety check to ensure at least one gyro remains enabled.
228-519: Well-implemented multi-gyro support with proper safety measures.The implementation successfully adds support for multiple gyros with:
- Dynamic UI generation based on detected gyros
- Proper enable/disable controls with safety checks
- Individual alignment settings for each gyro
- Backwards compatibility for older firmware versions
- Clear user feedback via
gui_logfor error conditionsThe code effectively handles the transition from scalar to array-based data structures while maintaining compatibility.
Also applies to: 709-750
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/tabs/configuration.js (1)
716-722: Fix gyro_align array initialization default value.Based on previous review feedback, the gyro_align array should be initialized with 1s instead of 0s to match the dropdown values (where 1 = "CW 0°").
- FC.SENSOR_ALIGNMENT.gyro_align = new Array(MAX_GYROS).fill(1); + FC.SENSOR_ALIGNMENT.gyro_align = new Array(MAX_GYROS).fill(1);Wait, this actually shows the code is already using
fill(1)which is correct. The previous review comment appears to have been addressed.
🧹 Nitpick comments (2)
src/js/tabs/configuration.js (2)
236-242: Improve gyro enable mask initialization logic.The safety check for
gyro_enable_mask === 0is good, but the initial condition check could be more robust.- if (FC.SENSOR_ALIGNMENT.gyro_enable_mask === undefined) { - FC.SENSOR_ALIGNMENT.gyro_enable_mask = (1 << MAX_GYROS) - 1; // All enabled by default - } else if (FC.SENSOR_ALIGNMENT.gyro_enable_mask === 0) { - // Safety check: if somehow all gyros are disabled, enable the first one - FC.SENSOR_ALIGNMENT.gyro_enable_mask = 1; - } + if (FC.SENSOR_ALIGNMENT.gyro_enable_mask === undefined || FC.SENSOR_ALIGNMENT.gyro_enable_mask === 0) { + // Initialize with all gyros enabled or enable first gyro if all were disabled + FC.SENSOR_ALIGNMENT.gyro_enable_mask = FC.SENSOR_ALIGNMENT.gyro_enable_mask === undefined + ? (1 << MAX_GYROS) - 1 + : 1; + }
264-441: Consider extracting large UI creation function for better maintainability.The
createGyroAlignmentUIfunction is quite large (177 lines) and handles multiple responsibilities. Consider breaking it down into smaller, focused functions.+ function createGyroBox(gyroIndex) { + return $(`<div class="gyro_box" id="gyro_box_${gyroIndex + 1}"></div>`); + } + + function createAlignmentControls(gyroIndex) { + // Move alignment dropdown creation logic here + } + + function createCustomAlignmentInputs(gyroIndex) { + // Move custom alignment inputs creation logic here + } + + function setupGyroEventHandlers(gyroIndex, alignmentSelect, enableCheck, customInputs) { + // Move all event handler setup logic here + } + function createGyroAlignmentUI(gyroIndex, container) { - // Large implementation + const gyroBox = createGyroBox(gyroIndex); + const { alignmentSelect, enableCheck } = createAlignmentControls(gyroIndex); + const customInputs = createCustomAlignmentInputs(gyroIndex); + + setupGyroEventHandlers(gyroIndex, alignmentSelect, enableCheck, customInputs); + + // Assembly logic }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(3 hunks)src/css/tabs/configuration.less(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(3 hunks)src/js/tabs/configuration.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
locales/en/messages.json (1)
undefined
<retrieved_learning>
Learnt from: haslinghuis
PR: #4490
File: locales/en/messages.json:1534-1545
Timestamp: 2025-05-26T11:37:09.741Z
Learning: In the Betaflight Configurator repository, only English messages should be added directly to locales/en/messages.json; other language translations are handled via Crowdin automation and should not be manually updated in PRs.
</retrieved_learning>
🔇 Additional comments (14)
src/js/fc.js (1)
481-484:✅ Verification successful
LGTM! Array initialization supports multi-gyro functionality.
The change from scalar zero initialization to empty arrays correctly enables support for multiple gyros as stated in the PR objectives. This aligns with the broader refactoring to array-based structures mentioned in the AI summary.
However, please verify that all dependent code has been updated to handle these properties as arrays rather than scalars, as this is a breaking change.
🏁 Script executed:
#!/bin/bash # Description: Verify all usage of the changed gyro alignment properties to ensure compatibility with array structure # Search for usage of the gyro alignment properties that are now arrays echo "=== Searching for gyro_align usage ===" rg -A 3 -B 1 "gyro_align[^_]" --type js echo "=== Searching for gyro_align_roll usage ===" rg -A 3 -B 1 "gyro_align_roll" --type js echo "=== Searching for gyro_align_pitch usage ===" rg -A 3 -B 1 "gyro_align_pitch" --type js echo "=== Searching for gyro_align_yaw usage ===" rg -A 3 -B 1 "gyro_align_yaw" --type jsLength of output: 19117
🏁 Script executed:
#!/bin/bash # Verify presence of legacy scalar gyro alignment properties echo "=== Searching for gyro_1_align usage ===" rg -n "gyro_1_align" --type js echo "=== Searching for gyro_2_align usage ===" rg -n "gyro_2_align" --type jsLength of output: 2274
Approve multi-gyro array initialization
All references to the new array-based properties (
gyro_align,gyro_align_roll,gyro_align_pitch,gyro_align_yaw) insrc/js/msp/MSPHelper.jsandsrc/js/tabs/configuration.jscorrectly handle them as arrays. The legacy scalar properties (gyro_1_align,gyro_2_align) remain untouched for backward compatibility and are used only in their original contexts.No breaking changes or missing updates were found.
src/js/msp/MSPHelper.js (3)
26-27: LGTM!The constant follows the existing naming convention and is appropriately placed.
643-671: Well-structured backward compatibility implementation.The code properly handles both the new array-based gyro alignment format for API 1.47+ and maintains support for older versions. The use of the
MAX_GYROSconstant ensures consistency.
2097-2127: Correctly implements the write operation for the new gyro alignment format.The implementation properly mirrors the read operation, maintaining the same data format and backward compatibility.
src/css/tabs/configuration.less (1)
57-133: Clean and well-structured CSS implementation.The styles are properly organized with good use of flexbox for layouts, consistent spacing, and appropriate hover/focus states. The nesting under
.tab-configurationensures proper scoping.locales/en/messages.json (3)
1534-1543: English-only i18n additions look correct
The three new keys—configurationSensorGyroEnable,configurationSensorAlignment, andconfigurationGyroRequiredWarning—are properly named, include valid English messages, and the dynamic$t(sensorStatusGyroShort.message)reference is protected by a"Don't translate!!!"description.
3155-3157: Dynamic gyro select key is correctly defined
ThemotorsSensorGyroSelectentry mirrors the established pattern by reusing thesensorStatusGyroShortmessage via$t(...)and includes the"Don't translate!!!"note to prevent translation errors.
4752-4754: Dynamic PID tuning gyro option is correctly defined
ThepidTuningItermRelaxTypeOptionGyroentry follows the same$t(sensorStatusGyroShort.message)pattern with an appropriate"Don't translate!!!"description, ensuring consistency across dynamic sensor labels.src/js/tabs/configuration.js (6)
14-14: LGTM: Added necessary logging utility import.The gui_log import is properly added and used appropriately for user-facing warning messages.
20-32: LGTM: Well-defined constants for sensor alignments and maximum gyros.The constants are clearly defined and will improve maintainability. The MAX_GYROS value of 8 aligns with the PR objectives for IMU3+ support.
398-415: Input validation implementation looks correct.The input validation for custom alignment values properly clamps values within the min/max bounds as suggested in previous reviews. This addresses potential issues with out-of-range values.
424-437: Excellent safety mechanism for preventing all gyros from being disabled.The logic correctly prevents users from disabling all gyros, which could render the flight controller inoperable. The immediate feedback via gui_log is user-friendly.
736-738: Good use of stored element references.The code correctly uses the stored
gyro_align_elementsarray references instead of complex DOM selectors, which addresses the robustness concern from previous reviews.
742-747:✅ Verification successful
Verify consistent gyro alignment data handling.
The save logic correctly gathers alignment values for all gyros without commented detection checks. This appears to address the previous review feedback about redundant commented code.
Let me verify that the gyro detection logic is consistently applied throughout the file:
🏁 Script executed:
#!/bin/bash # Check for any remaining commented gyro detection logic echo "=== Searching for commented detection logic ===" rg -n "//.*gyro.*detection" src/js/tabs/configuration.js echo "=== Searching for commented if statements around gyro logic ===" rg -A5 -B5 "//.*if.*gyro" src/js/tabs/configuration.jsLength of output: 2249
No further changes needed: gyro alignment and detection logic are correctly implemented.
The save loop now collects alignment values for all gyros, and there are no commented-out detection checks remaining in src/js/tabs/configuration.js. Ready to merge.
|
|
Preview URL: https://772b9957.betaflight-configurator.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/css/tabs/configuration.less (1)
57-133: Well-structured CSS for the gyro configuration UI.The styling is clean and follows good practices:
- Effective use of flexbox for responsive layouts
- Proper hover and focus states for better UX
- Consistent spacing and visual hierarchy
- Good naming conventions
Consider adding CSS transitions for smoother hover effects:
.gyro_box:hover { border-color: #bbb; box-shadow: 0 1px 4px rgba(0, 0, 0, 0.1); + transition: all 0.2s ease; }Also consider adding transitions to the input focus state:
.alignment_input_cell input:focus { border-color: #999; outline: none; + transition: border-color 0.2s ease; }src/js/tabs/configuration.js (2)
265-442: Consider breaking down this large function for better maintainability.The
createGyroAlignmentUIfunction is 177 lines long and handles multiple responsibilities. Consider extracting helper functions for:
- Creating alignment inputs (roll/pitch/yaw)
- Setting up event handlers
- Initializing values
This would improve readability and make the code easier to test and maintain.
Would you like me to help refactor this function into smaller, more focused functions?
745-747: Consider adding radix parameter to parseInt for consistency.While unlikely to cause issues with number inputs, it's a best practice to always specify the radix parameter for
parseInt.- FC.SENSOR_ALIGNMENT.gyro_align_roll[i] = parseInt($(`.gyro_${i + 1}_align_roll`).val()) || 0; - FC.SENSOR_ALIGNMENT.gyro_align_pitch[i] = parseInt($(`.gyro_${i + 1}_align_pitch`).val()) || 0; - FC.SENSOR_ALIGNMENT.gyro_align_yaw[i] = parseInt($(`.gyro_${i + 1}_align_yaw`).val()) || 0; + FC.SENSOR_ALIGNMENT.gyro_align_roll[i] = parseInt($(`.gyro_${i + 1}_align_roll`).val(), 10) || 0; + FC.SENSOR_ALIGNMENT.gyro_align_pitch[i] = parseInt($(`.gyro_${i + 1}_align_pitch`).val(), 10) || 0; + FC.SENSOR_ALIGNMENT.gyro_align_yaw[i] = parseInt($(`.gyro_${i + 1}_align_yaw`).val(), 10) || 0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
locales/en/messages.json(3 hunks)src/css/tabs/configuration.less(1 hunks)src/js/fc.js(1 hunks)src/js/msp/MSPHelper.js(3 hunks)src/js/tabs/configuration.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/fc.js
🧰 Additional context used
🧠 Learnings (1)
locales/en/messages.json (1)
undefined
<retrieved_learning>
Learnt from: haslinghuis
PR: #4490
File: locales/en/messages.json:1534-1545
Timestamp: 2025-05-26T11:37:09.741Z
Learning: In the Betaflight Configurator repository, only English messages should be added directly to locales/en/messages.json; other language translations are handled via Crowdin automation and should not be manually updated in PRs.
</retrieved_learning>
🔇 Additional comments (11)
locales/en/messages.json (3)
1534-1543: New English-only i18n keys for gyro settings
The keysconfigurationSensorGyroEnable,configurationSensorAlignment, andconfigurationGyroRequiredWarningare correctly introduced in the English locale. Per repository policy, only English messages should live in this file; Crowdin automation handles all other language translations.
3155-3157: Use dynamic sensor status label in motors UI
ThemotorsSensorGyroSelectentry now referencessensorStatusGyroShort.messageand includes a non-translatable description, aligning with the established pattern for dynamic UI labels.
4752-4754: Use dynamic sensor status label in PID tuning
ThepidTuningItermRelaxTypeOptionGyrokey is updated to use the dynamicsensorStatusGyroShort.messagealong with adescriptionto exclude it from translation, ensuring consistency across configurations.src/js/msp/MSPHelper.js (3)
26-26: Good use of constant definition.Defining
MAX_GYROSas a constant improves code maintainability and readability.
643-671: Well-implemented backward compatibility for sensor alignment data.The code properly handles both new (API >= 1.47) and legacy API versions:
- Correctly replaces individual gyro bytes with arrays and enable mask for newer API
- Maintains backward compatibility for older API versions
- Properly initializes arrays before reading data
- Correctly applies scaling factor when reading alignment values
Consider adding a comment explaining the purpose of the
/10scaling factor for future maintainers.
2103-2127: Consistent implementation of sensor alignment data serialization.The write logic correctly mirrors the read logic:
- Properly handles both API versions with appropriate data structures
- Correctly applies the inverse scaling factor (*10) when writing
- Maintains consistency with the data format expected by the firmware
src/js/tabs/configuration.js (5)
228-228: Useconstinstead ofletfor immutable array.The
gyro_align_elementsarray is never reassigned, only its elements are modified. Usingconstbetter expresses the intent and prevents accidental reassignment.- const gyro_align_elements = []; + const gyro_align_elements = [];Wait, it's already const in the code. Let me re-check...
Actually, line 228 shows:
const gyro_align_elements = [];So this is already using const. No issue here.
239-244: Good safety check for gyro enable mask initialization.The code properly handles the edge case where all gyros might be disabled, ensuring at least one gyro is always enabled for flight safety.
399-416: Proper input validation implemented.The code correctly validates custom alignment inputs against their min/max constraints and handles NaN cases with the
|| 0fallback. This ensures alignment values stay within valid ranges.
425-438: Excellent safety check preventing all gyros from being disabled.The implementation properly prevents users from creating a dangerous configuration with no active gyros, shows a clear warning message, and maintains the UI state correctly.
717-748: Well-implemented save logic with proper array handling.The code correctly:
- Initializes arrays with appropriate defaults (gyro_align with 1 for "CW 0°")
- Uses stored element references for reliable access
- Properly gathers all gyro configuration data
All past review concerns have been addressed.
haslinghuis
dismissed
coderabbitai[bot]’s stale review
Important changes have been made.
|
I don't have any hardware to verify this with. What's a good board I could request a sample of? |
Dual gyro IMU can also test this to an extent. |
|
Don't have dual IMU either as far as I know |
|
i would like to test. however, unknown schedule as my weekends are busy. i do have a 4-gyro and a 2-gyro, both mounted and flyable. |
|
@nerdCopter - for testing with H7dev you need to build locally using this config |
|
re: firmware: seems there should be an automated way of counting gyros rather than |
any changes to APEXF7 needed besides building with EDIT: thinking YES, because it only sees 1 and Success:
|
No, unless you want to set both IMU on by default or you want to have the second IMU be default. Otherwise it'll just default to the first IMU. |
i did do this: which was flown like 40 seconds or whatever. the logs were submitted on firmware PR. |
when testing will need to defineGYRO_COUNTin the build, i.e.GYRO_COUNT=2Summary by CodeRabbit