Skip to content

[1.4 backport] Allow using Navigator with the new lsm6dsv IMU#3945

Open
Williangalvani wants to merge 2 commits into
bluerobotics:1.4-devfrom
Williangalvani:new_ins_1.4
Open

[1.4 backport] Allow using Navigator with the new lsm6dsv IMU#3945
Williangalvani wants to merge 2 commits into
bluerobotics:1.4-devfrom
Williangalvani:new_ins_1.4

Conversation

@Williangalvani

@Williangalvani Williangalvani commented Jun 9, 2026

Copy link
Copy Markdown
Member

Summary by Sourcery

Update Navigator Pi4/Pi5 detection to handle multiple magnetometer options and extend device ID decoding with new compass and IMU types.

New Features:

  • Support alternative magnetometer devices for Navigator Pi4 and Pi5 detection, requiring at least one to be present alongside core sensors.
  • Recognize additional compass types in the frontend device ID decoder, including QMC5883P, BMM350, IIS2MDC, and LIS2MDL.
  • Recognize additional IMU types in the frontend device ID decoder, including ICM45686, SCHA63T, IIM42653, and LSM6DSV.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The NavigatorPi4 and NavigatorPi5 classes now share identical magnetometer_devices and very similar detect logic; consider extracting this into a shared helper or base implementation to reduce duplication and keep future changes consistent.
  • In deviceid_decoder.ts, the new IMU enum entries use slightly inconsistent spacing (e.g., double spaces before some =); aligning the formatting with existing enum members will improve readability.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `NavigatorPi4` and `NavigatorPi5` classes now share identical `magnetometer_devices` and very similar `detect` logic; consider extracting this into a shared helper or base implementation to reduce duplication and keep future changes consistent.
- In `deviceid_decoder.ts`, the new IMU enum entries use slightly inconsistent spacing (e.g., double spaces before some `=`); aligning the formatting with existing enum members will improve readability.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

Automated PR Review

0. Summary

  • Verdict: MINOR SUGGESTIONS ✏️

Extends Navigator Pi4/Pi5 detection so the magnetometer requirement is satisfied by any of a set of mags (currently AK09915 or IIS2MDC at 0x1E) rather than the AK09915 specifically, and adds new compass/IMU enum entries to the frontend deviceid_decoder.

6. Code Quality & Style

  • 6.1 [minor] core/frontend/src/utils/deviceid_decoder.ts:77 and :79INS_SCHA63T = 0x3C and INS_LSM6DSV = 0x3E use two spaces before = while every other enum member in the file (and the two siblings added in the same hunk) uses one. This is inconsistent and is the sort of thing no-multi-spaces / key-spacing from the airbnb base config flags. Drop one space on each line.
  • 6.2 [nit] core/services/ardupilot_manager/flight_controller_detector/linux/navigator.py:43-46 and :73-76magnetometer_devices is now identical between NavigatorPi5 and NavigatorPi4, and required_devices differs only in the PCA9685 bus number. Could be hoisted to a shared class attribute on a common base (or a module-level constant) to keep them in sync as more mags are added. Not a blocker — pre-existing duplication pattern.
  • 6.3 [nit] core/services/ardupilot_manager/flight_controller_detector/linux/navigator.py:45 and :75 — the dict key "IIS2MDC_0x1E" encodes the I2C address in the key while the address is also in the value tuple. If the intent is to distinguish multiple addresses of the same part (0x1E vs 0x1C), fine; otherwise just "IIS2MDC" would match the convention used by the other entries.

7. Tests

  • 7.1 [nit] No tests added, but detect() depends on real I2C bus probing via check_for_i2c_device, so the gap is consistent with existing coverage for these classes. Mentioned only for completeness.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

@Williangalvani Williangalvani changed the title New ins 1.4 [1.4 backport] Allow using Navigator with the new lsm6dsv IMU Jun 9, 2026
@patrickelectric

Copy link
Copy Markdown
Member

Just to be sure, is it ok for us to help this PR until 1.4.4 is released for RadCam ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants