Projects: ad2s1210: Add support for ad2s1210

[ahaslam2]

The AD2S1210 is a complete 10-bit to 16-bit resolution tracking resolver-to-digital converter

[CLAassistant]

All committers have signed the CLA.

[mphalke]

@ahaslam2 Please sign the CLA before we start reviewing this PR

[jansunil]

General question - EEPROM implementation is not seen. Are there plans to add it in the future?

[jansunil]

General comment - .gitignore file needs to be added within the projects/ad2s1210_iio directory Example- https://github.com/analogdevicesinc/precision-converters-firmware/blob/main/projects/ad4696_iio/.gitignore

[ahaslam2]

Change log of last push:

Fix comments from @jansunil:

• various blank spaces and lines fixes
• removed iio_types.h included by iio_trigger.h
• removed AD2S1210_ENABLE_HYSTERESIS defined in no-os driver
• moved ad2s1210_attribute_id to user defined section
• uses macros for ad2s1210_velocity_scale table
• removed iio_types.h and ad2s1210.h from ad2s1210_iio.h
• used gpio defined in mbed_config for ad2s1210_user_config
• add resolution macro in app_config
• define and use vcom_ops as done in ad738x
• add descriptor comments
• use macro limits for max count
• remove unused led_gpio_desc
• add comment about how sample rate was defined

[ahaslam2]

Change log of last push:

• Added brackets for single line if statements.

[ahaslam2]

Changes on last push

• use channel mask for single conversion
• less code in conversion loop depends on: https://github.com/analogdevicesinc/no-OS/pull/1953

[ahaslam2]

Change log from last push

• document fly wire setup in readme.
• added function doc for attr_set, attr_get and trigger_handler
• removed unneeded trigger_gpio_param
• reorder DATA_CAPTURE_MODE defines like other projects (ignore warnings)

@jansunil i think i addressed all the comments, https://github.com/analogdevicesinc/no-OS/pull/1953 was merged. if there are no more comments would it be ok to merge this PR now?

[jansunil]

The changes look good to me. @mphalke @ribdp - Do you have any comments on these commits?

[jansunil]

@ahaslam2 - Looks like some APIs are dependent on the latest no-OS driver commits from github.com/analogdevicesinc/no-OS/pull/1953

Should we also update the commit hash here?

[mphalke]

Change log from last push

• document fly wire setup in readme.
• added function doc for attr_set, attr_get and trigger_handler
• removed unneeded trigger_gpio_param
• reorder DATA_CAPTURE_MODE defines like other projects (ignore warnings)

@jansunil i think i addressed all the comments, analogdevicesinc/no-OS#1953 was merged. if there are no more comments would it be ok to merge this PR now?

I don't think all comments are addressed properly to merge this PR. Please check again and fix all

[ahaslam2]

@ahaslam2 - Looks like some APIs are dependent on the latest no-OS driver commits from github.com/analogdevicesinc/no-OS/pull/1953

Should we also update the commit hash here?

@jansunil sorry not sure what i should update. you mean i should update the commit hash of libraries/no-OS with the latest commit hash of no-os?

[ahaslam2]

change log:

• use no_os_memswap64 instead of swab macro. (@ribdp )

[jansunil]

@ahaslam2 - Looks like some APIs are dependent on the latest no-OS driver commits from github.com/analogdevicesinc/no-OS/pull/1953 Should we also update the commit hash here?

@jansunil sorry not sure what i should update. you mean i should update the commit hash of libraries/no-OS with the latest commit hash of no-os?

@ahaslam2 So the latest commit that has been made to the ad2s1210 no-OS drivers is here so the no-OS.lib file and its respective sub-module needs to be referenced "2627ec0bfe7f79ef3b0027b6a96ea2a4a7aa6d8c" as its commit ID so that the changes made as a part of this commit are also included when someone uses the FW.

A straightforward copy-paste of this ID would be enough for updating the no-OS.lib Please check here for some inputs on how to update the sub-modules.

[ahaslam2]

Changelog:

• updated no-os submodule commit hash to include dependent patch.

[ahaslam2]

@jansunil is it ok now ? Not sure what the CI build errors are about, they seem not related to this patch. I tested

• git clone --recursive https://github.com/analogdevicesinc/precision-converters-firmware
• git cherry-pick remotes/origin/ahaslam/ad2s1210_rc1 -1
• git submodule update --recursive --init

mbed-compile was OK.

[ahaslam2]

@jansunil @mphalke i added a patch in this MR to update to the latest version of the libs:

https://github.com/analogdevicesinc/precision-converters-firmware/pull/24/commits/8d787723bfbfed2e1ee70b91da07602549b1da42

I compiled with mbed-cli all seems ok would it be possible you take another look at this and let me know if its ok to merge?

[mphalke]

@ahaslam2 I had given some comments last month but unfortunately those were not get reflected into the PR. Apologies. Please check them now and do the necessary updates to code.

[mphalke]

@ahaslam2 Some more questions:

1. What's the plan on adding build and test support for this project? I think we should have them in place. Please refer any existing project and check how are adding build and test support (it's quite straightforward). We need to have ci_build.groovy file in the project directory and then pytest based tests folder.

2. I didn't see anything done w.r.t. eeprom validation and context attribute read/validation in the FW. Have a look at this code here: https://github.com/analogdevicesinc/precision-converters-firmware/blob/main/projects/ad4130_iio/app/ad4130_iio.c#L1692. It is important to have bord info validated and conveyed to client tool so that evaluation software detects the valid board. Have a look at this file created by Phillip which contains the checklist for firmware release. https://github.com/analogdevicesinc/precision-converters-firmware/blob/main/CONTRIBUTING.md. If you want to do that as a part of new PR, please do so.

3. There are few things which you may not be able to do such as updating ADI product pages, link wiki page to product page, etc. These things are part of release checklist (CONTRIBUTING.md). So I would suggest to prepare a list of items which are not done as per checklist and share with us, so that we know what is pending and if we can do from our side. I would be bit skeptical of the releasing project without this checklist.

4. I assume STM32 work is also planned and that would be added as a part of new PR

I will get back to you if anything is pending once above things are addressed. Thanks.

[ahaslam2]

Hi @mphalke,

Thanks for the feedback ill fix your comments.

about the questions:

1. ill check about adding build test support.
2. there is no eeprom validation because we use a breakout board to be able to plug in to the K1 this breakout board has its own eeprom on the same i2c buss. i was told that eeprom validation was not mandatory, but if really is, and the eeproms have different addresses i can check about adding this.
3. ill prepare list of items which are not done with respect to the checklist.
4. we will have to see about STM32 after we are done with this.

[mphalke]

@ahaslam2

1. Thanks
2. Understood. In that case we could just return the hardcoded hw_carrier and hw_mezzanine names from context attributes. FW version would be taken from our Jenkins build system. hw_carrier would be MCU board (e.g. SDP_K1), hw_mezzanine would be Eval board name that you are using.
3. Thanks
4. Having just Mbed support is fine now.

[ahaslam2]

Change log:

• only update no-os sha
• and test folder with python scripts
• add hard coded context attributes for hw_carrier, hw_mezzanine, hw_name, hw_vendor
• add more entries to .gitignore
• sync Makefile to newer projects
• style fixes
• Did not add ci_build.groovy as we cannot test/support this.

Notes: No EEPROM detection as we use breakout board with different EEPROM that gets auto detected by PCF CI_groovy was not validated. only test python script was run for the angle channel . wiki pages not updated.

• [X] Copyright header has been added to source and header files

• [X] Artistic Style (astyle) has been run to lint new code

  astyle --options=../no-OS/ci/astyle_config \
       projects/foobar/app/*.c \
       projects/foobar/app/*.h

• [ ] Add EEPROM validation code

• [X] Add all context attributes (e.g. fw_version, hw_carrier, hw_mezzanine, hw_name, hw_mezzanine_status)

• [X] IIO attributes in the firmware match the corresponding Linux driver attributes

• [X] Create a readme.md in the project directory explaining how to use the project

• [ ] Validate EEPROM detection and context attributes creation using IIO clients (e.g. ACE, IIO Oscilloscope)

• [X] Capture and verify ADC data using an IIO client

• [ ] Test on the target platform with USE_SDRAM and NO_SDRAM where applicable

• [X] Test on the target platform with physical and virtual serial ports where applicable

• [ ] Test using the STM32 HAL where applicable

• [ ] Validate attributes, calibration, temperature sensing, etc using a client

• [ ] Update corresponding ADI Wiki pages

• [ ] Request links to those pages on the product page

• [X] Open a pull request on Github

[ahaslam2]

Changelog:

• rename active_count to active_count
• use gpio_ops instead of mbed_gpio_ops
• remove print in conftest.py
• increase Copyright year
• limit test pass range to 2.5 and 3.5 rad.
• remove data sample loop in test_ad2s1210_example.py

[ahaslam2]

Change log:

• change ASZS-BREAKOUT to ADZS-BRKOUT in readme

[ahaslam2]

Change log:

• remove ad2s1210.py use the one in pyadi-iio
• use hw cs instead of sw cs
• fix another ASZS-BREAKOUT -> ASZS-BRKOUT in readme

[mphalke]

looks good to me unless any comments from @jansunil and @ribdp

[ahaslam2]

changelog:

• fix typo in readme doc