Support for generic ms5xxx barometric sensor #561
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
sensors/baro/ms5xxx/common.c
Outdated
|
|
||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
[clang-format-pr] reported by reviewdog 🐶
suggested fix
| /* | ||
| * Phoenix-RTOS | ||
| * | ||
| * Common part of TE Connectivity MS5xxx series barometric sensors. |
There was a problem hiding this comment.
[codespell] reported by reviewdog 🐶
TE ==> THE, BE, WE, TO
| /* | ||
| * Phoenix-RTOS | ||
| * | ||
| * Common part of TE Connectivity MS5xxx series barometric sensors. |
There was a problem hiding this comment.
[codespell] reported by reviewdog 🐶
TE ==> THE, BE, WE, TO
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @gmaz-phoesys, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone quickly understand the changes.
This PR aims to improve the sensor drivers for TE Connectivity MS5xxx series barometric sensors by extracting common functionality into shared files. This refactoring makes it easier to add support for new sensors in the series and reduces code duplication. Additionally, this PR adds a new driver specifically for the MS5525 differential barometric sensor.
Highlights
- Common MS5xxx Code: Introduces new files (
common.candcommon.h) containing shared functions and definitions for MS5xxx series sensors, such as CRC calculation, measurement routines, and hardware setup (reset, PROM reading, validation). - New MS5525 Driver: Adds a new driver (
ms5525.c) for the MS5525 differential barometric sensor, utilizing the newly created common MS5xxx functions for initialization and measurement. - MS5611 Driver Refactoring: Refactors the existing MS5611 driver (
ms5611.c) to use the common MS5xxx functions, removing duplicate code for hardware setup and measurement.
Changelog
Click here to see the changelog
- sensors/baro/ms5xxx/common.c
- New file: Implements common functions for MS5xxx sensors, including PROM CRC calculation (
ms5xxx_crc4), ADC measurement (ms5xxx_measure), and hardware initialization (ms5xxx_hwSetup).
- New file: Implements common functions for MS5xxx sensors, including PROM CRC calculation (
- sensors/baro/ms5xxx/common.h
- New file: Defines common commands (reset, conversion, read ADC, read PROM), OSR delays, the
ms5xxx_ctx_tcontext structure, and declares the common functions.
- New file: Defines common commands (reset, conversion, read ADC, read PROM), OSR delays, the
- sensors/baro/ms5xxx/ms5525.c
- New file: Adds the driver for the MS5525 differential barometric sensor.
- Implements
ms5525_publishthrto read pressure and temperature, perform calculations using sensor-specific coefficients, and publishSENSOR_TYPE_DIFFBAROevents. - Implements
ms5525_allocandms5525_startfor sensor registration and initialization, usingms5xxx_hwSetup.
- sensors/baro/ms5xxx/ms5611.c
- Refactored to include
common.h. - Updated
ms5611_publishthrto usems5xxx_measurefor reading ADC values. - Updated
ms5611_allocto usems5xxx_hwSetupfor hardware initialization.
- Refactored to include
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces common support for MS5xxx series barometric sensors and adds a new driver for the MS5525DSO differential pressure sensor. The refactoring into common files (common.c, common.h) is a good step towards modularity and reusability. The new MS5525 driver and updates to the MS5611 driver leverage this common code.
I've found a few issues, including a critical one related to data structure access in the MS5525 driver, and some high-severity concerns regarding SPI mode configuration and SPI function usage. There are also some medium-severity points about hardcoded values that might affect the generic applicability of the MS5525 driver.
Overall, the direction is good, but these issues should be addressed to ensure correctness and robustness.
Summary of Findings
- Critical Data Structure Access: In
ms5525.c,sensor_event_tmembers are accessed incorrectly (e.g.,ctx->evtDiaroinstead ofctx->evt.diffBaro), which will lead to undefined behavior or crashes. - SPI Mode Configuration: Both MS5525 and MS5611 drivers are configured for
SPI_MODE3. Datasheets for these sensors specifySPI_MODE0. This mismatch can cause communication failures. - SPI Function Usage for Reset: In
common.c, thespimsg_xferfunction might be used incorrectly for the sensor reset command due to howspiCtx.oidis populated. Usingsensorsspi_xferis recommended for consistency. - Hardcoded Values in MS5525 Driver: The MS5525 driver uses a hardcoded static pressure and a pressure range check specific to a high-pressure variant, which may limit its generic applicability.
- Copyright Year: The copyright year in new files is set to 2025. (Not commented due to review settings)
- Comment Typo: In
ms5xxx_crc4(common.c), there's a typo: "crc reminder" should be "crc remainder". (Not commented due to review settings) - Type Usage for Microsecond Delays: The
ms5xxx_measurefunction usestime_tfor theusDelayparameter, which is intended to be microseconds. While this might work, usinguseconds_toruint32_twould be more conventional and clearer. (Not commented due to review settings)
Merge Readiness
This pull request makes good progress in generalizing MS5xxx sensor support. However, due to the identified critical and high-severity issues, particularly the incorrect data structure access in ms5525.c and the SPI mode configurations, the code is not yet ready for merging. These issues must be addressed to ensure correct and reliable sensor operation. The medium-severity concerns regarding hardcoded values in the MS5525 driver should also be considered for improved reusability. I am unable to approve this pull request in its current state; please address the feedback and have the changes reviewed again.
sensors/baro/ms5xxx/ms5525.c
Outdated
| ctx->evtDiaro.timestamp = now; | ||
| ctx->evtDiffBaro.diffBaro.pressStatic = 101325; | ||
| ctx->evtDiffBaro.diffBaro.pressTotal = 101325 + press; | ||
|
|
||
| sensorspublish(info->id, &ctx->evtDiffBaro); |
There was a problem hiding this comment.
There seems to be a mix-up in accessing the sensor_event_t structure. The context ctx has a member evt of type sensor_event_t. When ctx->evt.type is SENSOR_TYPE_DIFFBARO, you should access its members like ctx->evt.timestamp and ctx->evt.diffBaro.pressStatic, etc.
The current usage of ctx->evtDiaro.timestamp and ctx->evtDiffBaro.diffBaro... will likely lead to incorrect memory access, data corruption, or crashes as evtDiaro and evtDiffBaro are not members of ms5xxx_ctx_t.
It should be:
ctx->evt.timestamp = now;
ctx->evt.diffBaro.pressStatic = 101325;
ctx->evt.diffBaro.pressTotal = 101325 + press;
sensors_publish(info->id, &ctx->evt); ctx->evt.timestamp = now;
ctx->evt.diffBaro.pressStatic = 101325; /* Assuming standard atmospheric pressure for static component */
ctx->evt.diffBaro.pressTotal = 101325 + press;
sensors_publish(info->id, &ctx->evt);
sensors/baro/ms5xxx/common.c
Outdated
|
|
||
| /* Reset ms5525 sequence */ | ||
| cmd = CMD_RESET; | ||
| if (spimsg_xfer(&ctx->spiCtx, &cmd, sizeof(cmd), NULL, 0, 0) < 0) { |
There was a problem hiding this comment.
The spimsg_xfer function typically expects ctx->oid within spimsg_ctx_t to be the specific slave device OID. However, ctx->spiCtx.oid is populated by sensorsspi_open with the SPI bus controller OID. This might lead to the reset command not being correctly addressed to the sensor slave.
Consider using sensorsspi_xfer for consistency and clarity, as it's designed to work with spiCtx (containing bus OID, mode, speed) and a separate spiSS (slave OID). It would look like:
sensorsspi_xfer(&ctx->spiCtx, &ctx->spiSS, &cmd, sizeof(cmd), NULL, 0, 0)
Could you clarify if spimsg_xfer is intended to be used this way, or if sensorsspi_xfer would be more appropriate here?
| if (spimsg_xfer(&ctx->spiCtx, &cmd, sizeof(cmd), NULL, 0, 0) < 0) { | |
| if (sensorsspi_xfer(&ctx->spiCtx, &ctx->spiSS, &cmd, sizeof(cmd), NULL, 0, 0) < 0) { |
| info->ctx = ctx; | ||
| info->types = SENSOR_TYPE_DIFFBARO; | ||
|
|
||
| ctx->spiCtx.mode = SPI_MODE3; |
There was a problem hiding this comment.
MS5xxx series sensors, including the MS5525, typically use SPI Mode 0 (CPOL=0, CPHA=0). Using SPI_MODE3 (CPOL=1, CPHA=1) might lead to communication errors or unreliable behavior depending on the SPI controller and sensor timing tolerances.
Could you verify the required SPI mode for MS5525 from its datasheet and adjust this accordingly? It's highly recommended to use the mode specified by the manufacturer.
ctx->spiCtx.mode = SPI_MODE0;| info->ctx = ctx; | ||
| info->types = SENSOR_TYPE_BARO; | ||
|
|
||
| ctx->spiCtx.mode = SPI_MODE3; |
There was a problem hiding this comment.
Similar to the MS5525, the MS5611 sensor typically uses SPI Mode 0 (CPOL=0, CPHA=0) as per its datasheet. Configuring SPI_MODE3 (CPOL=1, CPHA=1) might cause communication issues.
Please confirm the correct SPI mode for MS5611 and update if necessary. Adhering to the datasheet specification is crucial for reliable operation.
ctx->spiCtx.mode = SPI_MODE0;| press = ((((int64_t)D1 * sens) >> 21) - off) >> 15; /* P = ((D1 * SENS) / 2^21 - OFF) / 2^15 */ | ||
|
|
||
| /* MS5525DSO operating ranges: temperature from -40C to +80C, and pressure: 45000Pa, 110000 Pa */ | ||
| if (press > -206842 && press < 206842 && temp > -4000 && temp < 8500) { |
There was a problem hiding this comment.
The pressure range check press > -206842 && press < 206842 (approx +/- 30 PSI) is specific to certain MS5525DSO variants (e.g., MS5525DSO-DS030DS-N). If this driver is intended for generic MS5525DSO support, this hardcoded range might be too restrictive for lower-pressure variants or too permissive if a lower-pressure variant is mistakenly used.
Could this range be made configurable, or perhaps derived from PROM coefficients if possible (though unlikely for range itself)? If this driver targets a specific variant, a comment clarifying this would be helpful.
sensors/baro/ms5xxx/ms5525.c
Outdated
| /* MS5525DSO operating ranges: temperature from -40C to +80C, and pressure: 45000Pa, 110000 Pa */ | ||
| if (press > -206842 && press < 206842 && temp > -4000 && temp < 8500) { | ||
| ctx->evtDiaro.timestamp = now; | ||
| ctx->evtDiffBaro.diffBaro.pressStatic = 101325; |
There was a problem hiding this comment.
The static pressure pressStatic is hardcoded to 101325 Pa (standard atmospheric pressure). For a differential pressure sensor, the meaning of 'static' and 'total' pressure depends on the application. If this driver is for general use, this hardcoding might not always be appropriate.
Is this value always applicable for the intended use cases, or should it be configurable or obtained from another source?
gmaz-phoesys
force-pushed
the
gmaz_phoesys/generic_ms5xxx
branch
from
6dcd103 to
b66ea35
Compare
Description
These commits contain changes providing support for generic ms5xxx barometric sensors' series. New differential barometer type MS5525 is also added.
Motivation and Context
As new sensors from ms5xxx series appear in our projects it is worth to extract shared part into separate files.
Types of changes
How Has This Been Tested?
Checklist:
Special treatment