Skip to content

applet.sensor.qmc5883p: initial implementation#1122

Open
i-infra wants to merge 11 commits intoGlasgowEmbedded:mainfrom
i-infra:ii/qmc5883p
Open

applet.sensor.qmc5883p: initial implementation#1122
i-infra wants to merge 11 commits intoGlasgowEmbedded:mainfrom
i-infra:ii/qmc5883p

Conversation

@i-infra
Copy link
Copy Markdown

@i-infra i-infra commented Mar 6, 2026

Initial implementation for the qmc5883p magnetometer sensor - i2c @ 0x2C.

This code is tested working with the GY-271 breakout ie) https://www.aliexpress.us/item/2251832421287528.html.

Shoutout @adafruit (referenced their libs)

Datasheet for part @ https://www.qstcorp.com/upload/pdf/202512/2C939E5AA0704285BC3BE71132B8629B.pdf

PS: sending sm love to the glasgow team <3

…883p magnetometer sensor (i2c @ 0x2C) - tested working with GY-271 breakout ie) https://www.aliexpress.us/item/2251832421287528.html. shoutout adafruit (referenced their libs)
@i-infra i-infra requested a review from whitequark as a code owner March 6, 2026 02:12
Copy link
Copy Markdown
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

At a glance this looks great! The only issues I can see without closely looking through the functionality is ones that were already a problem in the codebase before you started working on your applet.

Please do the following in addition to inline comments:

  • Add yourself to CODEOWNERS for this applet
  • Add a documentation page at least for the CLI
  • Add types to the Interface class (if this is something you would normally use in your code)
  • Document the Interface class and add an API documentation section to the documentation page (I will merge the applet without this but it is preferred to have API docs for new applets like this one)

Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py
Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py Outdated
@whitequark
Copy link
Copy Markdown
Member

Regarding code formatting, there is a Ruff configuration and a pre-commit file that invokes it, this configuration is the one appropriate for Glasgow source code.

@whitequark whitequark added the waiting-on-author Status: Waiting on issue reporter or PR author label Mar 6, 2026
…, add docs, add types, switch to enums for parameters
@i-infra i-infra requested a review from whitequark March 6, 2026 02:41
Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py Outdated
Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py Outdated
Copy link
Copy Markdown
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

I did a rough pass (not a final one), looks better. By the way, I'm not sure why but "requesting review" from me on GitHub doesn't seem like it sends me an email; leaving a comment is the best way to get my attention.

…, add stub for rst autogen, add qmc5883 to index
@i-infra
Copy link
Copy Markdown
Author

i-infra commented Mar 7, 2026

I think it's good to go? Need to figure out legacy vs appletv2 stuff eventually, but check it out when you get a sec! Suspect it's approaching mergable.

@whitequark
Copy link
Copy Markdown
Member

Need to figure out legacy vs appletv2 stuff eventually, but check it out when you get a sec! Suspect it's approaching mergable.

Oh I somehow missed that you were still using the legacy applet infrastructure. New legacy applets are a big "no", I've been trying to migrate all of them to the new infrastructure for months already.

@i-infra
Copy link
Copy Markdown
Author

i-infra commented Apr 25, 2026

@whitequark I think this is good for re-review!

@i-infra i-infra requested a review from whitequark April 25, 2026 19:45
@whitequark
Copy link
Copy Markdown
Member

Thanks, will take a look once I'm done with some urgent bureaucracy.

Comment on lines +18 to +27
_CHIPID = 0x00
_XOUT_LSB = 0x01
_XOUT_MSB = 0x02
_YOUT_LSB = 0x03
_YOUT_MSB = 0x04
_ZOUT_LSB = 0x05
_ZOUT_MSB = 0x06
_STATUS = 0x09
_CONTROL1 = 0x0A
_CONTROL2 = 0x0B
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you also use an enum here?

Also, generally I avoid making things such as "register enumerations" IntEnums unless there's a specific reason to do so; accidentally doing something like _XOUT_LSB + 1 (in a more complex code path) and getting a nonsense result hurts a lot when debugging.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you're missing the enum here, unless it's intentional.

class Register(enum.Enum):
    CHIPID = 0x00
    XOUT_LSB = 0x01
    ...

Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py Outdated
Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py Outdated
Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py Outdated
Comment thread software/glasgow/applet/sensor/qmc5883p/__init__.py
@whitequark
Copy link
Copy Markdown
Member

This should be mergeable after the last review is addressed.

@whitequark
Copy link
Copy Markdown
Member

Ah sorry, missed two really important points I stated elsewhere:

  • needs documentation (you should include both the CLI docs as well as the Interface class and every enumeration it references; check that there aren't dead links in the generated docs)
  • needs tests (at a minimum an assertBuilds but I strongly prefer having record-replay tests for at least the basic functionality; should be easy here

…st.py, move regmap to enum.Enum, keep reg addy constants as plain ints - addresses review by @whitequark
Copy link
Copy Markdown
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Should be mergeable after the latest round of changs and maybe some tests!


.. module:: glasgow.applet.sensor.qmc5883p

.. autoexception:: QMC5883PError
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you're missing a lot of the newly added enumerations (the ones that now get surfaced in the public API).

Comment on lines +18 to +27
_CHIPID = 0x00
_XOUT_LSB = 0x01
_XOUT_MSB = 0x02
_YOUT_LSB = 0x03
_YOUT_MSB = 0x04
_ZOUT_LSB = 0x05
_ZOUT_MSB = 0x06
_STATUS = 0x09
_CONTROL1 = 0x0A
_CONTROL2 = 0x0B
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you're missing the enum here, unless it's intentional.

class Register(enum.Enum):
    CHIPID = 0x00
    XOUT_LSB = 0x01
    ...

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

Labels

waiting-on-author Status: Waiting on issue reporter or PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants