Skip to content

Conversation

@BortEngineerDude
Copy link

@BortEngineerDude BortEngineerDude commented Aug 2, 2025

User description

This should close #10744
I made this driver looking into the datasheet, and I don't have the QMC5833P device, so testing is mandatory before merging.


PR Type

Enhancement


Description

This description is generated by an AI tool. It may have inaccuracies

  • Add QMC5883P magnetometer driver support

  • Implement axis transformation for QMC5883L compatibility

  • Register new compass hardware in detection system

  • Configure I2C bus and device initialization


Diagram Walkthrough

flowchart LR
  A["QMC5883P Driver"] --> B["Device Detection"]
  B --> C["I2C Communication"]
  C --> D["Data Reading"]
  D --> E["Axis Transformation"]
  E --> F["QMC5883L Compatibility"]
Loading

File Walkthrough

Relevant files
Enhancement
5 files
bus.h
Add QMC5883P device hardware identifier                                   
+1/-0     
compass_qmc5883p.c
Implement complete QMC5883P magnetometer driver                   
+202/-0 
compass_qmc5883p.h
Add QMC5883P driver header file                                                   
+27/-0   
compass.c
Integrate QMC5883P into compass detection system                 
+14/-0   
compass.h
Add QMC5883P to magnetometer sensor enum                                 
+1/-0     
Configuration changes
4 files
common_hardware.c
Register QMC5883P I2C bus device configuration                     
+7/-0     
common_post.h
Enable QMC5883P in magnetometer compilation flags               
+1/-0     
CMakeLists.txt
Add QMC5883P source files to build                                             
+2/-0     
settings.yaml
Add QMC5883P to magnetometer hardware settings                     
+1/-1     

@MATEKSYS
Copy link
Contributor

MATEKSYS commented Aug 4, 2025

compared QMC5883L and QMC5883P on M10Q-5883 board
Maybe we can set the alignment of QMC5883P to be the same as that of QMC5883L.
This way, users will not be confused when setting it up.

QQ截图20250804131849
QQ截图20250804133818

@BortEngineerDude BortEngineerDude marked this pull request as ready for review August 4, 2025 17:56
@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 4, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
⚡ Recommended focus areas for review

Typo in Constant

Line 63 contains a typo in the constant name QMC5833P_CONF1_MODE_SINGLE which should be QMC5883P_CONF1_MODE_SINGLE to match the chip name pattern used throughout the file.

#define QMC5833P_CONF1_MODE_SINGLE     0x02
#define QMC5883P_CONF1_MODE_CONTINUOUS 0x03
Typo in Constant

Line 100 contains a typo in the constant name QMC5833P_STATUS_OVFL_MASK which should be QMC5883P_STATUS_OVFL_MASK to match the chip name pattern used throughout the file.

#define QMC5833P_STATUS_OVFL_MASK 0x02
Missing Y-axis Reset

In the qmc5883pRead function, only X and Z axes are reset to zero on line 132-133, but the Y-axis is not reset. This could leave stale data in magADCRaw[Y] if the read operation fails.

mag->magADCRaw[X] = 0;
mag->magADCRaw[Z] = 0;

@qodo-merge-pro
Copy link

qodo-merge-pro bot commented Aug 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Reset all magnetometer axes
Suggestion Impact:The suggestion was directly implemented - the missing Y-axis reset line was added exactly as suggested

code diff:

+    mag->magADCRaw[Y] = 0;

The Y-axis is not being reset to zero in case of failed read, which could leave
stale data. All three axes should be reset to maintain consistency and prevent
potential issues with outdated magnetometer readings.

src/main/drivers/compass/compass_qmc5883p.c [131-133]

 // set magData to zero for case of failed read
 mag->magADCRaw[X] = 0;
+mag->magADCRaw[Y] = 0;
 mag->magADCRaw[Z] = 0;

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where only the X and Z axes are cleared, potentially leaving stale data in the Y axis upon a failed read, which could lead to incorrect sensor fusion.

Medium
  • Update

@MATEKSYS
Copy link
Contributor

MATEKSYS commented Aug 5, 2025

QMC5883P alignment is same as QMC5883L now.

@sensei-hacker
Copy link
Collaborator

It would be great if someone can test this.

@synersignart
Copy link

It would be great if someone can test this.

im on it, what do you need I have a controller here with the QMC5883P.

@synersignart
Copy link

image image

Right now its detecting the Mag but wasnt able to read data off it. I need more info from BortEngineer on this

@synersignart
Copy link

compared QMC5883L and QMC5883P on M10Q-5883 board Maybe we can set the alignment of QMC5883P to be the same as that of QMC5883L. This way, users will not be confused when setting it up.

QQ截图20250804131849 QQ截图20250804133818

any update on your end regarding the QMC5883P drivers'?

@MATEKSYS
Copy link
Contributor

any update on your end regarding the QMC5883P drivers'?

image

@synersignart
Copy link

image image image image image

Manage to confirm it from BortEngineerDude from another independent test of the mag.

@synersignart
Copy link

after a few more test we can push this on the next INAV release

@sensei-hacker
Copy link
Collaborator

sensei-hacker commented Oct 16, 2025

@synersignart
Thanks for doing that testing!

You had said "Right now its detecting the Mag but wasnt able to read data off it"
From your latest message, does that mean this PR is now working properly for you? Based on your testing, this is ready to merge?

@synersignart
Copy link

synersignart commented Oct 16, 2025

w working properly for you? Based on your testing, this is

its currently working on my flight controller as is now.
and near ready thanks to BortEngineer assistance. whats remain is do we need to mount it on a drone to test it?

excuse for having it ported to the INAV 8.0.1 as I need the configurator to visualize it at the moment

@sensei-hacker
Copy link
Collaborator

excuse for having it ported to the INAV 8.0.1 as I need the configurator to visualize it at the moment

A compatible Configurator can be found at
https://github.com/iNavFlight/inav-configurator/actions/runs/18541410810/artifacts/4281504999 (Windows)

https://github.com/iNavFlight/inav-configurator/actions/runs/18541410810/artifacts/4281431148 (Linux)

whats remain is do we need to mount it on a drone to test it?

I don't think there is any need for that - just need to check that it correctly reads north, south, and east and west on the bench.

@synersignart
Copy link

synersignart commented Oct 16, 2025

Ive tested it too with the compass 0-360 readout at setup its showing correctly so far I can reconfirm it again later when I get home

@sensei-hacker
Copy link
Collaborator

sensei-hacker commented Oct 16, 2025

Thank you very much, all of you!

@sensei-hacker sensei-hacker merged commit 2f78152 into iNavFlight:master Oct 16, 2025
@synersignart
Copy link

image so far I can confirm on several bench test that the compass heading is all good 0-N 90-E 180-S 270-W

@MrD-RC MrD-RC added this to the 9.0 milestone Oct 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add QMC5883P support

5 participants