drivers: sensor: add bma400 accelerometer driver - #104178
Conversation
|
Hello @lucagessi, and thank you very much for your first pull request to the Zephyr project! |
fcdbb71 to
2f26e80
Compare
|
Hi! Following up on this PR since all CI checks are green. Let me know if you have any questions. |
|
Hi @MaureenHelm, Just a quick follow-up since the CI checks are all green. Is there anything else you need me to address before review? I'm also happy to split this into smaller commits if the current one is too large. Let me know what you think—I'm open to any changes. Thanks! |
I took a quick look and saw two things:
I'll try to come back soon and do a more in-depth review. |
2f26e80 to
430dfd8
Compare
6b7d437 to
14ec47c
Compare
|
@MaureenHelm I made the changes you suggested. |
|
|
Hi @MaureenHelm @teburd, I implemented the requested changes. All checks are green. Let me know if everything is ok now!🙏 |
e00c3c6 to
4a1706a
Compare
|
|
@ubieda I made the changes you asked. Are ok for you? |
|
Hi, @lucagessi I'll take another look in the next couple of days! Thanks for your patience. |
| depends on SENSOR_ASYNC_API | ||
| select SPI if $(dt_compat_on_bus,$(DT_COMPAT_BOSCH_BMA400),spi) | ||
| select SPI_RTIO if $(dt_compat_on_bus,$(DT_COMPAT_BOSCH_BMA400),spi) | ||
| select RTIO_WORKQ |
There was a problem hiding this comment.
Let's drop this as it isn't required
| struct bma400_decoder_header header; | ||
| uint8_t int_status; | ||
| uint16_t accel_odr: 4; | ||
| uint16_t fifo_count: 10; |
There was a problem hiding this comment.
IIUC from the datasheet - The FIFO can hold 1024 bytes. Don't we, then, need 11-bits to hold the max value?
| const bool has_accel = (BMA400_FIFO_EMPTY_FRAME & buffer[0]) != 0; | ||
| const bool is_empty = (BMA400_FIFO_EMPTY_FRAME == buffer[0]); |
There was a problem hiding this comment.
I'm confused because I'm failing to see how has_accel can be false and is_empty true at the same time (See if-elseif-else branching below). Please explain the intend.
In general: please double-check the header classification and properly handle the flags critical for the features supported.
| struct rtio_iodev_sqe *streaming_sqe = drv_data->streaming_sqe; | ||
|
|
||
| ARG_UNUSED(sqe); | ||
| ARG_UNUSED(result); |
There was a problem hiding this comment.
Handle result, as it may indicate a failure (applies to all completion cb's)
| uint8_t value; | ||
| uint8_t power_mode_conf = 0; | ||
|
|
||
| /* If ODR is greater than 25Hz, enable normal mode */ |
There was a problem hiding this comment.
Would you mind clarifying how we enforce this comment? Is it stale?
There was a problem hiding this comment.
I Will remove It. The sensor works only in normal mode or low power with auto sleep.
There was a problem hiding this comment.
Ok - In general: I advise to look over the driver and simplify it to cover the features that are supported. I'd rather work with a driver that is limited functionality but reliable than one that has many features but not sure what is well-supported and what isn't.
There was a problem hiding this comment.
I understand your point. I needed these features for a project so I tried to implement: auto wake up, auto sleep and motion event.
There was a problem hiding this comment.
Is not clear how it works? Do I need to say it clearly?
There was a problem hiding this comment.
No need to ellaborate: If you need all the features that's okay!
I'd simply suggest to step back, take another pass at the PR and address what you consider could use some improvement (e.g: const usage, error-handling, variables-sizing, etc).
There was a problem hiding this comment.
Ok. I will do a review of the code by myself taking into account your suggestions. Thank you very much ubieda, I'll do my best ;)
| /** | ||
| * Set the sensor's acceleration threshold for motion detection | ||
| */ | ||
| static int bma400_attr_set_motion_threshold(const struct sensor_value *val, |
There was a problem hiding this comment.
Should this be SI (e.g: the same unit as SENSOR_CHAN_ACCEL_XYZ)?
| /** | ||
| * Set the sensor's acceleration motion event duration | ||
| */ | ||
| static int bma400_attr_set_motion_duration(const struct sensor_value *val, |
There was a problem hiding this comment.
Should this be SI (as in: Seconds)?
| int (*write_reg)(const struct device *dev, uint8_t reg_addr, uint8_t value); | ||
| }; | ||
|
|
||
| struct bma400_runtime_config { |
There was a problem hiding this comment.
Please clairfy why the following fields should be constant:
- auto_wakeup_en
- auto_lowpower_en
- int1_map
- int2_map
The reason I ask is because this whole struct is updated at run-time.
| struct rtio_sqe *write_reset_fifo = rtio_sqe_acquire(r); | ||
| const uint8_t write_buffer[] = { | ||
| BMA400_REG_CMD, | ||
| 0xb0, /* Reset FIFO */ |
There was a problem hiding this comment.
Why not using BMA400_FIFO_FLUSH_CMD?
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
|
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note that if it gets closed, you can ask someone to reopen it for you if you do not have the permissions to do so. |
|
Sorry for the delay, been busy with work. I'm resuming work on the driver now. |
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note that if it gets closed, you can ask someone to reopen it for you if you do not have the permissions to do so. |
Add initial support for the Bosch BMA400 accelerometer. The existing bma4xx driver is not compatible with the BMA400 due to significant register map differences. Supported features: - On-motion auto wakeup - Inactivity auto low-power mode - Motion detection trigger (normal mode) - FIFO data streaming (normal mode) Signed-off-by: Luca Gessi <lucagessi90@gmail.com>
Add bma400 sensor to build test. Signed-off-by: Luca Gessi <lucagessi90@gmail.com>
4a1706a to
26d63e2
Compare
|
@ubieda I made some changes to the driver. Below main changes: Single interrupt line (INT1)
Sensor API conformance (review feedback)
FIFO decoding fixes
RTIO error and lifecycle handling
Cancellation support (new)
Other
|



Add initial support for the Bosch BMA400 accelerometer. The existing bma4xx driver is not compatible with the BMA400 due to significant register map differences.
Supported features:
#85462 highlights that bma4xx driver is not compatible with bma400.
#8139 asks for bma400 driver implementation