Frontend: stop renaming RCIN9/RCIN10 if Sub version is higher than 4.5.4#3860
Open
Williangalvani wants to merge 1 commit intobluerobotics:masterfrom
Open
Frontend: stop renaming RCIN9/RCIN10 if Sub version is higher than 4.5.4#3860Williangalvani wants to merge 1 commit intobluerobotics:masterfrom
Williangalvani wants to merge 1 commit intobluerobotics:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds version-aware handling of ArduSub RCIN channel name mappings so that RCIN9/RCIN10 are only labeled as Lights 1/2 for firmware <= 4.5.4, preventing confusing renames on newer versions, by introducing an effective_param_value_map computed property and using semver comparison. Class diagram for updated PwmSetup Vue componentclassDiagram
class PwmSetup {
+string name
+Dictionary~Dictionary~string~~ param_value_map
+Dictionary~string~ legacy_sub_param_value_map
+boolean is_sub
+string vehicle_type
+Dictionary~Dictionary~string~~ effective_param_value_map
+Parameter[] servo_function_parameters()
+string stringToUserFriendlyText(text string)
+Parameter getParam(param string)
}
class Autopilot {
+FirmwareInfo firmware_info
}
class FirmwareInfo {
+string version
}
class Semver {
+boolean lte(versionA string, versionB string)
}
PwmSetup --> Autopilot : uses_autopilot
Autopilot --> FirmwareInfo : has
PwmSetup ..> Semver : uses_lte_in_effective_param_value_map
class ParamValueMaps {
+Dictionary~Dictionary~string~~ param_value_map
+Dictionary~string~ legacy_sub_param_value_map
+Dictionary~Dictionary~string~~ effective_param_value_map
}
PwmSetup *-- ParamValueMaps : manages_channel_labels
File-Level Changes
Possibly linked issues
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:
- Defaulting the firmware version to '0.0.0' in
sem_ver_lte(autopilot.firmware_info?.version ?? '0.0.0', '4.5.4')will cause the legacy RCIN9/RCIN10 mapping to be applied when the version is unknown; consider skipping the legacy behavior if the version is missing or unparsable instead. - To make the legacy cutoff clearer and easier to maintain, consider extracting the '4.5.4' version string into a named constant (e.g.,
LEGACY_SUB_LIGHTS_MAX_VERSION) near thelegacy_sub_param_value_mapdefinition and reusing it in thesem_ver_ltecheck.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Defaulting the firmware version to '0.0.0' in `sem_ver_lte(autopilot.firmware_info?.version ?? '0.0.0', '4.5.4')` will cause the legacy RCIN9/RCIN10 mapping to be applied when the version is unknown; consider skipping the legacy behavior if the version is missing or unparsable instead.
- To make the legacy cutoff clearer and easier to maintain, consider extracting the '4.5.4' version string into a named constant (e.g., `LEGACY_SUB_LIGHTS_MAX_VERSION`) near the `legacy_sub_param_value_map` definition and reusing it in the `sem_ver_lte` check.
## Individual Comments
### Comment 1
<location path="core/frontend/src/components/vehiclesetup/PwmSetup.vue" line_range="237-239" />
<code_context>
}
},
computed: {
+ effective_param_value_map(): Dictionary<Dictionary<string>> {
+ const map = { ...param_value_map }
+ if (this.is_sub && sem_ver_lte(autopilot.firmware_info?.version ?? '0.0.0', '4.5.4')) {
+ map.Submarine = { ...legacy_sub_param_value_map, ...map.Submarine }
+ }
</code_context>
<issue_to_address>
**issue:** Defaulting to firmware version '0.0.0' risks misclassifying unknown or unparsable versions as legacy.
Using `'0.0.0'` as the fallback means any missing/unparsable `autopilot.firmware_info?.version` is treated as `<= 4.5.4`, so unknown versions silently get legacy behavior. That can hide issues when the version field is wrong or new firmware doesn’t match expectations. Consider either skipping the legacy map when the version is falsy/unparsable, or guarding `sem_ver_lte`/defaulting to a clearly non-legacy version so unknown versions don’t inherit legacy lights by default.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ES-Alexander
requested changes
Mar 31, 2026
Collaborator
ES-Alexander
left a comment
There was a problem hiding this comment.
Code generally looks ok, though I haven't tested.
A couple of important comments:
| computed: { | ||
| effective_param_value_map(): Dictionary<Dictionary<string>> { | ||
| const map = { ...param_value_map } | ||
| if (this.is_sub && sem_ver_lte(autopilot.firmware_info?.version ?? '0.0.0', '4.5.4')) { |
Collaborator
There was a problem hiding this comment.
Suggested change
| if (this.is_sub && sem_ver_lte(autopilot.firmware_info?.version ?? '0.0.0', '4.5.4')) { | |
| if (this.is_sub && sem_ver_lte(autopilot.firmware_info?.version ?? '0.0.0', '4.5.6')) { |
According to the release notes and the tag diff, lights funcs were added in 4.5.6.
Member
Author
There was a problem hiding this comment.
bumped to 4.5.5, as we are using lower-than-equals
21f7003 to
1d3ec80
Compare
1d3ec80 to
d9ac496
Compare
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.
We moved away from using RCIN9 and RCIN10 in 4.5.5. This renaming could then confuse users.
Summary by Sourcery
Conditionally map legacy RCIN9/RCIN10 labels for Sub vehicles based on firmware version to avoid incorrect PWM port naming in newer ArduSub releases.
Bug Fixes:
Enhancements: