[1.4-backport] only rename RCIN9/RCIN10 when relevant#3875
Merged
patrickelectric merged 1 commit intobluerobotics:1.4from Apr 15, 2026
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideBackports logic to only rename RCIN9/RCIN10 channel functions for ArduSub vehicles running firmware versions up to 4.5.5 by introducing a version-aware parameter map and semver-based comparison, ensuring newer firmware that handles lights natively is unaffected. Class diagram for updated PwmSetup parameter mapping logicclassDiagram
class PwmSetup {
+effective_param_value_map() Dictionary
+servo_function_parameters() Parameter[]
+stringToUserFriendlyText(text string) string
}
class ParamValueMaps {
+param_value_map Dictionary
+legacy_sub_param_value_map Dictionary
+LEGACY_SUB_LIGHTS_MAX_VERSION string
}
class SemVer {
+sem_ver_lte(version string, maxVersion string) bool
}
PwmSetup --> ParamValueMaps : uses
PwmSetup --> SemVer : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The comment on
LEGACY_SUB_LIGHTS_MAX_VERSIONsays ArduSub <= 4.5.4 but the constant is set to '4.5.5' and used withsem_ver_lte, which includes 4.5.5; please either adjust the version or the comment/condition so they are consistent with the actual boundary you intend. - In
effective_param_value_map, the logic branches onautopilot.firmware_info?.version, which is read from an imported singleton rather than Vue-reactive state; if the firmware version can change at runtime, consider wiring this through a reactive source so the computed map updates correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The comment on `LEGACY_SUB_LIGHTS_MAX_VERSION` says ArduSub <= 4.5.4 but the constant is set to '4.5.5' and used with `sem_ver_lte`, which includes 4.5.5; please either adjust the version or the comment/condition so they are consistent with the actual boundary you intend.
- In `effective_param_value_map`, the logic branches on `autopilot.firmware_info?.version`, which is read from an imported singleton rather than Vue-reactive state; if the firmware version can change at runtime, consider wiring this through a reactive source so the computed map updates correctly.
## Individual Comments
### Comment 1
<location path="core/frontend/src/components/vehiclesetup/PwmSetup.vue" line_range="205-206" />
<code_context>
},
} as Dictionary<Dictionary<string>>
+// ArduSub <= 4.5.4 used RCIN9/RCIN10 for lights control; newer versions handle lights natively
+const LEGACY_SUB_LIGHTS_MAX_VERSION = '4.5.5'
+const legacy_sub_param_value_map: Dictionary<string> = {
+ RCIN9: 'Lights 1',
</code_context>
<issue_to_address>
**issue:** Align the legacy lights comment with the version boundary constant or adjust the boundary.
`LEGACY_SUB_LIGHTS_MAX_VERSION` is `'4.5.5'` and used with `sem_ver_lte`, so the legacy mapping currently applies through 4.5.5, not 4.5.4 as the comment suggests. Please either change the constant to `'4.5.4'` or update the comment so they describe the same version range.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6550963. Configure here.
6550963 to
afa77b1
Compare
patrickelectric
approved these changes
Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Backport of #3860.
Summary by Sourcery
Make PWM setup treat RCIN9/RCIN10 as light controls only for legacy ArduSub firmware versions while preserving existing behavior for other vehicles and newer firmware.
New Features:
Bug Fixes:
Note
Low Risk
Low risk UI-labeling change gated to ArduSub firmware versions
<= 4.5.5; main risk is incorrect version parsing/comparison causing mislabeling.Overview
Makes PWM setup’s user-friendly parameter naming firmware-version aware for ArduSub.
RCIN9/RCIN10are now labeled as Lights 1/2 only when connected to legacy ArduSub firmware (<= 4.5.5), while newer ArduSub versions and other vehicle types keep their existing naming behavior.Reviewed by Cursor Bugbot for commit afa77b1. Bugbot is set up for automated code reviews on this repo. Configure here.