drivers: sensor: ams_as5600: Rewrite driver to enhance configuration #86532
Conversation
|
Hello @c-ewing, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
asemjonovs,
avisconti,
jeppenodgaard,
MaureenHelm,
teburd,
tristan-google,
ubieda and
yperess
| @@ -1,12 +1,172 @@ | |||
| # AS5600 Angular position sensor configuration option | |||
|
|
|||
| # Copyright (c) 2022, Felipe Neves. | |||
| # Copyright (c) 2025, Cameron Ewing | |||
There was a problem hiding this comment.
I think the common practice is to add a second copyright line
There was a problem hiding this comment.
This file was reverted as part of moving the wrongly placed configuration options and this should be resolved. The only other instance of removing his copyright was also fixed in ams_as5600.c.
| config AS5600_ANGULAR_RANGE | ||
| int "Maximum angular range" | ||
| range 205 4096 | ||
| default 4096 | ||
| help | ||
| Maximum angular range (min. 205; max. 4096) | ||
| The minimum angular range is 18 degrees (205 steps). |
There was a problem hiding this comment.
Is this not inferred from end - start?
There was a problem hiding this comment.
Although true that the angular range can be computed from the end and start positions, the AS5600 datasheet explicitly documents a method that uses a start position and an angular range to configure the device. To fully support all programming options as described by the manufacturer, I’ve retained the angular range setting in the driver. For reference, please see Figure 26 (Option C: Programming a Maximum Angular Range Through the I²C Interface on page 24 of the datasheet.
That being said I don't use the start and angular range configuration in my setup and am willing to remove it or directly document the different programming options to provide more clarity on its use.
There was a problem hiding this comment.
Lets see what this looks like when using devicetree bindings and take it from there.
| config AS5600_START_POSITION | ||
| int "Start position" | ||
| default 0 | ||
| range 0 4095 | ||
| help | ||
| Start position (min. 0; max. 4095) | ||
| Difference between Start and End position must be at least 18 degrees (205 steps). | ||
| config AS5600_END_POSITION | ||
| int "End position" | ||
| default 0 | ||
| range 0 4095 | ||
| help | ||
| End position (min. 0; max. 4095) | ||
| Difference between Start and End position must be at least 18 degrees (205 steps). | ||
|
|
||
| config AS5600_ANGULAR_RANGE | ||
| int "Maximum angular range" | ||
| range 205 4096 | ||
| default 4096 | ||
| help | ||
| Maximum angular range (min. 205; max. 4096) | ||
| The minimum angular range is 18 degrees (205 steps). |
There was a problem hiding this comment.
All of these don't seem to belong in Kconfig, what if I wanted 2 of these sensors with different ranges? I think they belong in devicetree.
There was a problem hiding this comment.
Almost all these kconfigs belong as devicetree bindings since we don't want them to affect every instance of this sensor.
There was a problem hiding this comment.
This is corrected and retested in the latest commit.
I started working on the driver with device tree configuration and then somewhere along the lines got my wires crossed and moved it all to Kconfig. Sorry!
c-ewing
force-pushed
the
as5600-driver-improvement
branch
from
23b285e to
889f21c
Compare
c-ewing
changed the title
This is a complete rewrite of the existing AS5600 driver. Device registers and configuration are supported through the fetch and get API using channels and attributes. The device tree binding was updated to cover all configurable options of the device. The device is now initialized to the state defined in the device tree on init or its power on reset state if a setting is not configured. Signed-off-by: Cameron Ewing <cameronramsayewing@gmail.com>
c-ewing
force-pushed
the
as5600-driver-improvement
branch
from
889f21c to
cd65082
Compare
|
I believe I’ve addressed all the initial concerns mentioned above. When you have time, I would greatly appreciate a second review of my proposed changes. Thanks again for your time, patience, and guidance. |
This is a complete rewrite of the existing AS5600 driver. Device registers and configuration are supported through the fetch and get API using channels and attributes. The device tree binding was updated to cover all configurable options of the device. The device is now initialized to the state defined in the device tree on init or its power on reset state if a setting is not configured.
This rewrite referenced the AS5600 datasheet and has been tested against hardware.
This is my first time touching Zephyr and I would greatly appreciate any and all feedback! I understand large PRs are not recommended but I wasn't sure how to split this up and am certainly willing to adjust and split this into multiple PRs.