Dev/admt4000 #2462
Dev/admt4000 #2462
Conversation
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| /***************************************************************************//** | ||
| * @file admt4000.c | ||
| * @brief Implementation of ADMT4000 Driver. | ||
| * @author Marc Sosa (marcpaolo.sosa@analog.com) | ||
| * @author Jose Ramon San Buenaventura (jose.sanbuenaventura@analog.com) | ||
| ******************************************************************************** | ||
| * Copyright 2024(c) Analog Devices, Inc. | ||
| * | ||
| * All rights reserved. | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions are met: | ||
| * - Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * - Redistributions in binary form must reproduce the above copyright | ||
| * notice, this list of conditions and the following disclaimer in | ||
| * the documentation and/or other materials provided with the | ||
| * distribution. | ||
| * - Neither the name of Analog Devices, Inc. nor the names of its | ||
| * contributors may be used to endorse or promote products derived | ||
| * from this software without specific prior written permission. | ||
| * - The use of this software may or may not infringe the patent rights | ||
| * of one or more patent holders. This license does not release you | ||
| * from the requirement that you obtain separate licenses from these | ||
| * patent holders to use this software. | ||
| * - Use of the software either in source or binary form, must be run | ||
| * on or directly connected to an Analog Devices Inc. component. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY ANALOG DEVICES "AS IS" AND ANY EXPRESS OR | ||
| * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, NON-INFRINGEMENT, | ||
| * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. | ||
| * IN NO EVENT SHALL ANALOG DEVICES BE LIABLE FOR ANY DIRECT, INDIRECT, | ||
| * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT | ||
| * LIMITED TO, INTELLECTUAL PROPERTY RIGHTS, PROCUREMENT OF SUBSTITUTE GOODS OR | ||
| * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER | ||
| * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, | ||
| * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| *******************************************************************************/ |
There was a problem hiding this comment.
The No-OS license has changed to BSD 3 clause. Use that instead. Example here: https://github.com/analogdevicesinc/no-OS/blob/main/include/no_os_crc16.h
| /******************************************************************************/ | ||
| /***************************** Include Files **********************************/ | ||
| /******************************************************************************/ |
There was a problem hiding this comment.
We don't use these comments for new contributions.
| #include "no_os_util.h" | ||
| #include "no_os_alloc.h" | ||
| #include "no_os_delay.h" | ||
|
|
There was a problem hiding this comment.
This whole files uses 4 spaces indentation instead of tabs.
| if (ret < 0) | ||
| goto err; |
| goto err; | ||
|
|
||
| /** Set Direction **/ | ||
| ret |= no_os_gpio_direction_output(dev->gpio_coil_rs, |
There was a problem hiding this comment.
You're checking the error code right after the function call, so there is no need to use the | operator.
|
|
||
| /* Angle Factor Conversion */ | ||
| for (i = 0; i < 2; i++) | ||
| angle[i] = (float)raw_angle[i] * admt4000_angle_conv_factors[i]; |
There was a problem hiding this comment.
We avoid the use of floating point numbers in the driver layer. Can we use radians instead of degrees here, and then return the angle as milliradians?
There was a problem hiding this comment.
The API where this code belongs to do not have references and was not also exposed. I might remove this instead.
667f775 to
a298529
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Make sure to fix astyle, cppcheck and documentation complaints. |
| #define ADMT4000_AGP_REG_ANGLESEC 0x08 | ||
|
|
||
| /*ADMT 0x00 Page Registers*/ | ||
| #define ADMT4000_RAW_ANGLE_REG(x) 0x10 + (x) |
There was a problem hiding this comment.
this one and many others are not properly aligned
There was a problem hiding this comment.
also parenthesize the whole expression
| if (ret) | ||
| return ret; | ||
|
|
||
| *angle = (float)raw_temp * admt4000_angle_conv_factors[1]; |
There was a problem hiding this comment.
rework all these computations in integer logic as not all hardware that will run this code has floating point unit and if float is software emulated it can be very slow
| #define ADMT4000_TURN_CNT_TWOS 64 | ||
|
|
||
| /* Conversion Factors */ | ||
| #define ADMT4000_RADIUS_RES 0.000924 // mV/V unit |
There was a problem hiding this comment.
you will have to define these multiplied by 1M or 10M (see my other comment about float)
| return 0; | ||
|
|
||
| err: | ||
| no_os_free(dev); |
There was a problem hiding this comment.
this is not enough, you have to make sure to deinit whatever has successfully passed the init prior to the failure point
| goto err; | ||
|
|
||
| /* Delay */ | ||
| no_os_mdelay(10); |
There was a problem hiding this comment.
explain the 10ms in a comment with reference to datasheet preferably
| /* Set to 0 */ | ||
| if (i == ((1 << k) - 1)) | ||
| { | ||
| code[(i / 8)] &= (uint8_t)~NO_OS_BIT((i % 8)); |
There was a problem hiding this comment.
i % 8 is functionally equivalent to i & 0x7, only that the latter is much more efficient
Given that you have it used a lot i'd suggest to change it to my proposal. Now, i & 0x7 is harder to read so make sure to document at least one of these usages where you briefly explain the reason for using it.
| } | ||
|
|
||
| //Assignment of value bits inside the code array | ||
| /* parity_num is in BITS */ |
There was a problem hiding this comment.
These 2 comments have inconsistent style
| if (value % 2) | ||
| value = 1; | ||
| else | ||
| value = 0; |
There was a problem hiding this comment.
this whole section you could replace with value &= 0x1;
| { | ||
| for (j = i; j < i + position; j++) | ||
| { | ||
| bit_extract = no_os_field_get(NO_OS_BIT((j % 8)), code[(j / 8)]); |
There was a problem hiding this comment.
see, this is exactly the kind of place where % operator should be optimized if possible, inefficiency is multiplied by the number of loops... so use the trick i told you about to optimize it
| if (ret) | ||
| return ret; | ||
| if (direction != NO_OS_GPIO_IN) | ||
| return ret; // TODO: Should return relevant info |
There was a problem hiding this comment.
i agree, it should return relevant info :D
1303904 to
fc4906e
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
39d1d88 to
238e02f
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
099a6e6 to
54f61ed
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Please follow this document and make sure you use the |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| ret = no_os_spi_remove(device->spi_desc); | ||
| if (ret) | ||
| return ret; | ||
|
|
There was a problem hiding this comment.
you need to remove the gpio descriptors as well
| return 0; | ||
|
|
||
| err: | ||
| admt4000_remove(dev); |
There was a problem hiding this comment.
this would only work if you do something like this in admt4000_remove:
if (device->spi_desc) {
ret = no_os_spi_remove(device->spi_desc);
if (ret)
return ret;
device->spi_desc = NULL;
}
... and do this for all gpios, and whatever else was allocated ...
Otherwise you need to create several goto labels like we do for other device drivers.
| if (ret) | ||
| return ret; | ||
| if (direction != NO_OS_GPIO_OUT) | ||
| return ret; // TODO: Should return relevant info |
There was a problem hiding this comment.
is this TODO not doable in this pull request ?
| if (ret) | ||
| return ret; | ||
|
|
||
| no_os_mdelay(5); |
There was a problem hiding this comment.
can you also document this delay ? is it arbitrary or from datasheet ?
| if (ret) | ||
| return ret; | ||
| if (direction != NO_OS_GPIO_IN) | ||
| return EIO; |
There was a problem hiding this comment.
in no-OS we always return negative error codes on errors, so -EIO
| } | ||
|
|
||
| //Assignment of value bits inside the code array | ||
| /* parity_num is in BITS */ |
There was a problem hiding this comment.
merge these or rearrange somehow to have consistent commit style
| if (no_os_field_get(ADMT4000_RCV_CRC, buf[3]) != excess) | ||
| return -EBADMSG; | ||
|
|
||
| if (buf[3] != NULL) |
There was a problem hiding this comment.
if (!buf[3])
return -EINVAL;
*verif = buf[3]
Also these checks can be made right after the spi transfer.
| return -EINVAL; | ||
|
|
||
| return admt4000_read(dev->admt4000_desc, (uint8_t)reg, | ||
| (uint16_t *) readval, NULL); |
There was a problem hiding this comment.
Casting readval to a pointer of different size will result in different values depending on the CPU endianess. You need to use an intermediate variable:
uint16_t temp;
admt4000_read(..., &temp);
*readval = temp;
| case IIO_ROT: // absangle | ||
| vals[0] = 360; | ||
| vals[1] = 10; |
There was a problem hiding this comment.
For rot and angl channel types, the IIO ABI defines the units as radians (https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-iio). Is your driver returning angles or radians here?
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@Lcompo can you summarize what changed in this version with a changelog ? |
|
Still WIP and consolidating changes. Thanks for the inputs |
|
ok, also make sure to rebase on |
The ADMT4000 is a zero power, multiturn sensor that can count up to 46 turns of an external magnetic field and report the absolute position via SPI. Signed-off-by: Louijie Compo <louijie.compo@analog.com>
This adds tinyIIO support for the ADMT4000 device. This exposes output angular measurements in channels. Signed-off-by: Louijie Compo <louijie.compo@analog.com>
Add initial project files and IIO example for ADMT4000. Signed-off-by: Louijie Compo <louijie.compo@analog.com>
Add README.rst documentation file for project alongside other documentation related files. Signed-off-by: Louijie Compo <louijie.compo@analog.com>
|
|
@Lcompo , i wanted to merge but looks like there is a doc conflict, can you please fix it ? |
|
|
||
| /******************************************************************************/ | ||
| /********************** Macros and Constants Definitions **********************/ | ||
| /******************************************************************************/ |
There was a problem hiding this comment.
also can you please remove all these code comment blocks from this project commit ?
Pull Request Description
Please replace this with a detailed description and motivation of the changes.
You can tick the checkboxes below with an 'x' between square brackets or just check them after publishing the PR.
If this PR contains a breaking change, list dependent PRs and try to push all related PRs at the same time.
PR Type
PR Checklist