add ad5293 drivers #2361

mi-zeng · 2024-11-09T15:33:03Z

Pull Request Description

drivers includes

no-os initilization
calibration, wiper r/w ,hard/soft reset and other operation
daisy-chain multiple chip support

has formatted according to #2352 code style with astyle.sh

PR Type

Bug fix (change that fixes an issue)
New feature (change that adds new functionality)
Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

I have followed the Coding style guidelines
I have performed a self-review of the changes
I have commented my code, at least hard-to-understand parts
I have build all projects affected by the changes in this PR
I have tested in hardware affected projects, at the relevant boards
I have signed off all commits from this PR
I have updated the documentation (wiki pages, ReadMe etc), if applies

kister-jimenez · 2024-11-12T06:34:06Z

/AzurePipelines run

azure-pipelines · 2024-11-12T06:34:25Z

Azure Pipelines successfully started running 2 pipeline(s).

amiclaus

there are some build issues that need addressing in the first place. have a look at #2351 for example on how documentation should be integrated.

mi-zeng · 2024-11-13T13:13:44Z

Hi,
I have ref to #2351, modified related sphinx source file and a make html is build successful, please take a look.
best

buha · 2024-11-15T08:03:59Z

drivers/potentiometer/ad5293/ad5293.c

+	}
+
+	*device = dev;
+	printf("Initializing succeed.\r\n ");


no need to put these messages in the device driver

buha · 2024-11-15T08:04:45Z

drivers/potentiometer/ad5293/ad5293.c

+ *
+ * @param dev - The device structure.
+ *
+ * @return NAN


buha · 2024-11-15T08:06:07Z

drivers/potentiometer/ad5293/ad5293.c

+		goto error;
+
+	// spi initialization
+	init_param->spi_init.mode = NO_OS_SPI_MODE_1; //CPOL=0, CPHA=1


the comment is redundant, but as you wish

buha · 2024-11-15T08:08:52Z

drivers/potentiometer/ad5293/ad5293.c

+	chp = (struct ad5293_chip_info*)no_os_malloc(dev->chip_num * sizeof(* chp));
+	if (! chp) {
+		ret = - ENOMEM;
+		goto error;


if you jump from here to error label, execution will attempt to free the unallocated chp pointer which is undefined behavior

will add explicity initilazation to pointer dev and chp as NULL, then it won't affect even if chp is not allocated a address

that works too

buha · 2024-11-15T08:11:15Z

drivers/potentiometer/ad5293/ad5293.c

+	if (! dev)
+		return - ENOMEM;
+
+	printf("Initializing ad5293, number of chips %d.\r\n", init_param->chip_num);


remove all printf statements from the device driver, they only belong to the application

you may use debug pr_* statements from no_os_print_log.h, but not printf directly

will be removed

buha · 2024-11-15T08:24:36Z

drivers/potentiometer/ad5293/ad5293.h

+SOFTWARE.
+*******************************************************************************/
+
+#ifndef _ad5293_H_


lower/upper case mix

buha · 2024-11-15T08:25:34Z

drivers/potentiometer/ad5293/ad5293.h

+
+#ifndef _ad5293_H_
+#define _ad5293_H_
+/*****************************************************************************/


delete these comment delimiters, they are completely useless

i know we have a lot of them in our code but we should get rid of them

buha · 2024-11-15T08:26:25Z

drivers/potentiometer/ad5293/ad5293.h

+};
+
+/* SDO mode */
+enum SDO_mode_t {


lower/uppser case mix

buha · 2024-11-15T08:27:54Z

drivers/potentiometer/ad5293/README.rst

+1.  Serial peripheral interface (SPI) is necessary to access device
+2.  Daisy-chain mode is required for multiple chip use, in this configuration 
+    chip select line remains low untill all bytes are shifted out
+3.  High impedence mode of SDO pin is not recommanded, 


this sentence needs rephrasing

buha · 2024-11-15T08:32:35Z

drivers/potentiometer/ad5293/README.rst

+3.  High impedence mode of SDO pin is not recommanded, 
+    as tested somehow this could cause communications 
+    unfunctional
+


also i think more details are needed in the documentation

i just searched a random device driver .rst to use as template:
https://github.com/analogdevicesinc/no-OS/blob/main/drivers/adc/adm1177/README.rst

amiclaus · 2024-11-15T09:12:01Z

drivers/potentiometer/ad5293/ad5293.c

+{
+	struct ad5293_dev* dev;
+	struct ad5293_chip_info* chp;
+	int32_t ret = 0;


no need for explicit initialization.

will remove = 0

amiclaus · 2024-11-15T09:12:27Z

drivers/potentiometer/ad5293/ad5293.c

+
+	// hardware reset pin initialization
+	if (init_param->gpio_reset) {
+		ret |= no_os_gpio_get_optional(& dev->gpio_reset, init_param->gpio_reset);


check ret after each assignment. avoid propagating errors with |=

amiclaus · 2024-11-15T09:12:48Z

drivers/potentiometer/ad5293/ad5293.c

+	if (init_param->gpio_reset) {
+		ret |= no_os_gpio_get_optional(& dev->gpio_reset, init_param->gpio_reset);
+		ret |= no_os_gpio_set_value(dev->gpio_reset, NO_OS_GPIO_HIGH);
+		if (ret < 0) {


if (ret) check all occurences.

amiclaus · 2024-11-15T09:14:34Z

drivers/potentiometer/ad5293/ad5293.h

+#include "no_os_util.h"
+
+/* SPI Read/Write commands */
+#define CMD_NOP        0b0000


go for hex values here entirely.

amiclaus · 2024-11-15T09:14:56Z

drivers/potentiometer/ad5293/ad5293.h

+
+/*******************************functions prototype*******************************/
+/* Initialize the ad5293 device structure. */
+int32_t ad5293_init(struct ad5293_dev **device,


all functions should return int

Do I have to return an error code?
I returned data in last 2 get operation functions

mi-zeng · 2024-11-17T14:15:39Z

11.17
several change according to review

reorgnize document README.rst, add more description
fix void function return description
turn ret initialization as non-explicit
remove printf information
check ret for each ret assignment, simplify if(ret<0) as if(ret)
chp init as NULL to prevent undefined memory release behavior
simplify return and function return in same line
fix lower/upper case mix
remove redundant delimiters
marco define turn form binary to hex

mi-zeng · 2024-11-18T02:17:25Z

any build error information from LinuxBuilds cppcheck?

buha · 2024-11-18T08:51:33Z

here's the log if you dont have access to it:

drivers/potentiometer/ad5293/ad5293.c:149:6: error: Uninitialized variable: ret [uninitvar]
 if (ret)
     ^
drivers/potentiometer/ad5293/ad5293.c:313:9: error: Uninitialized variable: ret [uninitvar]
 return ret;
        ^
drivers/potentiometer/ad5293/ad5293.c:338:9: error: Uninitialized variable: ret [uninitvar]
 return ret;
        ^

1. no-os initilization 2. calibration, wiper r/w ,hard/soft reset and other operation 3. signle chip/daisy-chain multiple chips support Signed-off-by: Mize <[email protected]>

mi-zeng · 2024-11-18T15:46:37Z

11.18

fix build error related code
add chip num check in init function
add struct member memory free in initilazation error part
add single chip setup description

mi-zeng · 2024-11-18T15:47:37Z

BTY I found the access to check logs, thanks :>

mi-zeng · 2024-11-20T15:02:13Z

HI，I guess there is something wrong with the Mbed projects chkecks. Since #20241118.5 on pipline the Mbed failed several time. So maybe someone gonna fix it and recheck this PR afterwards?

buha

yes, the mbed issue seems unrelated, i'm ok with merging like this, that will be fixed by someone else

mi-zeng requested review from buha, CiprianRegus, dbogdan, amiclaus, rbolboac and danmois as code owners November 9, 2024 15:33

amiclaus reviewed Nov 12, 2024

View reviewed changes

mi-zeng force-pushed the ad5293 branch from 6066695 to 791f772 Compare November 13, 2024 13:09

buha reviewed Nov 15, 2024

View reviewed changes

amiclaus reviewed Nov 15, 2024

View reviewed changes

mi-zeng force-pushed the ad5293 branch from 791f772 to 88a821f Compare November 17, 2024 14:13

add ad5293 drivers include

39bbc31

1. no-os initilization 2. calibration, wiper r/w ,hard/soft reset and other operation 3. signle chip/daisy-chain multiple chips support Signed-off-by: Mize <[email protected]>

mi-zeng force-pushed the ad5293 branch from 88a821f to 39bbc31 Compare November 18, 2024 15:45

buha approved these changes Nov 25, 2024

View reviewed changes

amiclaus approved these changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ad5293 drivers #2361

add ad5293 drivers #2361

mi-zeng commented Nov 9, 2024

kister-jimenez commented Nov 12, 2024

azure-pipelines bot commented Nov 12, 2024

amiclaus left a comment

mi-zeng commented Nov 13, 2024

buha Nov 15, 2024

buha Nov 15, 2024

buha Nov 15, 2024

buha Nov 15, 2024

mi-zeng Nov 17, 2024

buha Nov 18, 2024

buha Nov 15, 2024

mi-zeng Nov 17, 2024

buha Nov 15, 2024

buha Nov 15, 2024

buha Nov 15, 2024

buha Nov 15, 2024

buha Nov 15, 2024

amiclaus Nov 15, 2024

mi-zeng Nov 17, 2024

amiclaus Nov 15, 2024

amiclaus Nov 15, 2024

amiclaus Nov 15, 2024

amiclaus Nov 15, 2024

mi-zeng Nov 17, 2024

mi-zeng commented Nov 17, 2024

mi-zeng commented Nov 18, 2024

buha commented Nov 18, 2024

mi-zeng commented Nov 18, 2024

mi-zeng commented Nov 18, 2024

mi-zeng commented Nov 20, 2024

buha left a comment

add ad5293 drivers #2361

Are you sure you want to change the base?

add ad5293 drivers #2361

Conversation

mi-zeng commented Nov 9, 2024

Pull Request Description

PR Type

PR Checklist

kister-jimenez commented Nov 12, 2024

azure-pipelines bot commented Nov 12, 2024

amiclaus left a comment

Choose a reason for hiding this comment

mi-zeng commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mi-zeng commented Nov 17, 2024

mi-zeng commented Nov 18, 2024

buha commented Nov 18, 2024

mi-zeng commented Nov 18, 2024

mi-zeng commented Nov 18, 2024

mi-zeng commented Nov 20, 2024

buha left a comment

Choose a reason for hiding this comment