Skip to content

drivers/qma6100p: add QST QMA6100P 3-axis accelerometer driver#22352

Open
leduclean wants to merge 1 commit into
RIOT-OS:masterfrom
leduclean:qma6100p-driver-release
Open

drivers/qma6100p: add QST QMA6100P 3-axis accelerometer driver#22352
leduclean wants to merge 1 commit into
RIOT-OS:masterfrom
leduclean:qma6100p-driver-release

Conversation

@leduclean

Copy link
Copy Markdown

Add a driver for the QST QMA6100P 3-axis accelerometer over I2C.

This driver was needed to support the Seeed Studio SenseCAP T1000-E tracker,
which embeds a QMA6100P as its motion sensor.

Features:

  • Configurable full-scale range, output data rate and power mode
  • Acceleration readout on all three axes
  • Data-ready interrupt support
  • SAUL integration
  • Kconfig configuration

Includes a test application under tests/drivers/qma6100p covering basic
readout and the data-ready interrupt.

Testing procedure

Wire a QMA6100P to a board exposing an I2C bus (e.g. the Seeed Studio SenseCAP
T1000-E), then build and flash the test application (adjust BOARD and the I2C
parameters as needed):

BOARD=<your-board> make -C tests/drivers/qma6100p flash term

The test:

  1. Initializes the device over I2C.
  2. Runs a data-ready interrupt rate sweep at 12.5 / 25 / 100 Hz, measuring the
    ISR frequency and asserting it matches the configured ODR.
  3. Streams samples: the data-ready ISR signals a reader thread that reads each
    sample over I2C.

Expected output:

=== QMA6100P accelerometer driver test ===
[init] I2C_DEV(0) addr 0x12 ... OK

--- data-ready interrupt rate sweep ---
[ODR 1/3] set rate -> 12 Hz ... OK
  try 1/3: measured 12 Hz (expect ~12) -> PASS
[ODR 1/3] PASS (~12 Hz)
[ODR 2/3] set rate -> 25 Hz ... OK
  try 1/3: measured 25 Hz (expect ~25) -> PASS
[ODR 2/3] PASS (~25 Hz)
[ODR 3/3] set rate -> 100 Hz ... OK
  try 1/3: measured 100 Hz (expect ~100) -> PASS
[ODR 3/3] PASS (~100 Hz)

=== ALL TESTS PASSED ===

--- interrupt-driven streaming ---
[data] x=... y=... z=... ug
...

Issues/PRs references

#22267

Useful link

Declaration of AI Tools / LLMs usage

  • Claude Code for code review and guidance, with user authorship of the implementation

Co-authored-by: Baptiste Le Duc baptiste.leduc@etik.com
Signed-off-by: Léandre Le Duc leandre.leduc38@gmail.com

Add a driver for the QST QMA6100P 3-axis accelerometer over I2C.

Features:
- Configurable full-scale range, output data rate and power mode
- Acceleration readout on all three axes
- Data-ready interrupt support
- SAUL integration
- Kconfig configuration

Includes a test application under tests/drivers/qma6100p covering
basic readout and the data-ready interrupt.

Co-authored-by: Léandre Le Duc <leandre.leduc38@gmail.com>
Signed-off-by: Baptiste Le Duc <baptiste.leduc@etik.com>
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

Hey @leduclean, thank you for your first contribution! We really appreciate it! If you haven't already, please take a look at our contributing guidelines before the review process starts. Also, due to how the GitHub review system works, please avoid force-pushing or squashing your commits unless asked to by a maintainer (or unless your commit is still in "draft commit" stage). Your pull request will be reviewed as soon as possible.

@github-actions github-actions Bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer Area: Kconfig Area: Kconfig integration labels Jun 6, 2026
@crasbe crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 6, 2026
@riot-ci

riot-ci commented Jun 6, 2026

Copy link
Copy Markdown

Murdock results

FAILED

f13dc60 drivers/qma6100p: add QST QMA6100P 3-axis accelerometer driver

Success Failures Total Runtime
3173 0 10422 04m:08s

Artifacts

@AnnsAnns

AnnsAnns commented Jun 7, 2026

Copy link
Copy Markdown
Member

First Sanji and now Foxy, seems like there is a theme here 😛

Thank you for your PR, it seems like there are still some issues you need to fix to get through our CI, see here: https://ci.riot-os.org/details/b17c1a96704744cbb4adb7763282d497

I'd presume that @baptleduc can vouch for the functionality of this?

@baptleduc

Copy link
Copy Markdown
Contributor

First Sanji and now Foxy, seems like there is a theme here 😛

Thank you for your PR, it seems like there are still some issues you need to fix to get through our CI, see here: https://ci.riot-os.org/details/b17c1a96704744cbb4adb7763282d497

I'd presume that @baptleduc can vouch for the functionality of this?

Ahah brothers fans of one piece 😄

Yes I will fix it but @leduclean already took a look and he struggled to find what was the real issue

@baptleduc

baptleduc commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

It is weird because the murdock error indicates it is because of map_regs variable that could be used uninitialized in qma6100p_set_data_ready_interrupt() while there are no branches where it could be the case.

Could you please help us to understand this ?

Thanks !

@AnnsAnns

AnnsAnns commented Jun 7, 2026

Copy link
Copy Markdown
Member

Hmmm does this also happen when you run the test locally or only on murdock?

@crasbe

crasbe commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

The compiler complains about the use of map_reg in the function qma6100p_set_data_ready_int. In fact, the function _set_intpin_conf won't set the value of map_reg in the default case.

It is possible that the compiler can't resolve the goto conditions properly and therefore assumes that map_reg might be used uninitialized.

In general, I think the use of goto and the READ_REG macros that also get the goto label as a parameter is not really a good style tbh.

@AnnsAnns

AnnsAnns commented Jun 7, 2026

Copy link
Copy Markdown
Member

It is possible that the compiler can't resolve the goto conditions properly

The compiler once it sees goto

@baptleduc

baptleduc commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

In general, I think the use of goto and the READ_REG macros that also get the goto label as a parameter is not really a good style tbh.

You are right, but where do you see such usage ? You mean that we shouldn't pass the goto label as parameter of READ_REG macros ? So it will make the READ_REG/WRITE_REG macro useless

(void)args;
irq_count++;
if (reader_pid != KERNEL_PID_UNDEF) {
thread_flags_set(thread_get(reader_pid), FLAG_DATA_READY);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not just use a mutex?

* @param[in] dev device descriptor of accelerometer
* @param[out] data raw ADC counts per axis
*
* @return QMA6100P_NODATA if nothing has changed and keep data unchanged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

QMA6100P_NODATA is not defined

Comment on lines +86 to +90
if (res == -ENXIO) {
return QMA6100P_NODEV;
}
if (res == -EINVAL) {
return QMA6100P_INVALID_ARG;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why use custom error codes here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is a wrapper as one of the maintainer suggest when I did the ads1x1x driver

/* Wait for OTP to load */
do {
READ_REG(QMA6100P_REG_NVM, nvm_status, out);
} while ((nvm_status & (QMA6100P_NVM_LOAD_DONE | QMA6100P_NVM_RDY)) !=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

better add a limit on the number of retries

* @brief Interrupt configuration parameters
*/
typedef struct {
gpio_t interrupt_pin; /**< MCU GPIO connected to the QMA6100P INT pin */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be part of qma6100p_params_t

typedef struct {
gpio_t interrupt_pin; /**< MCU GPIO connected to the QMA6100P INT pin */
qma6100p_int_active_level_t active_level_int; /**< active level of INT pin */
qma6100p_int_pin_mode_t pin_mode_int; /**< INT pin output mode */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why would you need this to be configurable?

gpio_t interrupt_pin; /**< MCU GPIO connected to the QMA6100P INT pin */
qma6100p_int_active_level_t active_level_int; /**< active level of INT pin */
qma6100p_int_pin_mode_t pin_mode_int; /**< INT pin output mode */
qma6100p_int_shadow_t interrupt_shadow; /**< shadow mode */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does this mean?

qma6100p_int_active_level_t active_level_int; /**< active level of INT pin */
qma6100p_int_pin_mode_t pin_mode_int; /**< INT pin output mode */
qma6100p_int_shadow_t interrupt_shadow; /**< shadow mode */
qma6100p_int_pin_num_t interrupt_pin_num; /**< QMA6100P INT pin routed on the board */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess that means you can have multiple interrupt pins for different events?

*
* Work mode is controlled by PM_REGISTER (0x11) and can be set through I2C commands
*
* @warning By default, QMA6100P is in intermediate state after power on but it shouldn't be keep for any applications

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then there should only be a qma6100p_set_low_power(bool) right?

*
* @warning The callback is invoked from interrupt context, keep it short
*/
int qma6100p_set_data_ready_int(qma6100p_t *dev, const qma6100p_int_t *interrupt);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess the more idiomatic way would be

Suggested change
int qma6100p_set_data_ready_int(qma6100p_t *dev, const qma6100p_int_t *interrupt);
int qma6100p_set_data_ready_int(qma6100p_t *dev, const qma6100p_int_params_t *config, qma6100p_int_cb_t cb, void *arg);

*/
typedef enum {
QMA6100P_INT_CFG_SHADOW_EN = 0, /**< shadowing enabled (default) */
QMA6100P_INT_CFG_SHADOW_DIS = 1, /**< shadowing disabled */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does that even mean?

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

Labels

Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: SAUL Area: Sensor/Actuator Uber Layer Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants