Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnalogIn test issue #11746

Closed
yarbcy opened this issue Oct 24, 2019 · 32 comments

Comments

@yarbcy
Copy link
Contributor

@yarbcy yarbcy commented Oct 24, 2019

Description of defect

FPGA test: AnalogIn test issue.
Steps to reproduce:

  1. mbed test --run -m CY8CKIT_062_WIFI_BT -t GCC_ARM -n tests-mbed_hal_fpga_ci_test_shield-analogin
  2. Test failed
  3. Comment out line in analoin/main.c "Case("AnalogIn - init test", all_ports<AnaloginPort, DefaultFormFactor, analogin_init>),"
  4. Run tests
  5. Test PASS
  6. Uncomment 3), comment out next test case
  7. Run test
  8. Test PASS

Target(s) affected by this defect ?

tested on CY8CKIT_062_WIFI_BT

Toolchain(s) (name and version) displaying this defect ?

tested on GCC_ARM

What version of Mbed-os are you using (tag or sha) ?

Latest

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

N/A

How is this defect reproduced ?

Always.

@ciarmcom

This comment has been minimized.

Copy link
Member

@ciarmcom ciarmcom commented Oct 24, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Oct 25, 2019

cc @ARMmbed/team-cypress Please review

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 28, 2019

@0xc0170 This issue is reported by Cypress team.

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Oct 28, 2019

I don't understand where is the issue. What should be expected and what is failing in the init phase? can you clarify more details as you already looked at the failures.

cc @ARMmbed/mbed-os-hal

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 28, 2019

@0xc0170 I run analogin tests.

main.cpp contains two test cases:
Case cases[] = {
// This will be run for all pins
Case("AnalogIn - init test", all_ports<AnaloginPort, DefaultFormFactor, analogin_init>),
// This will be run for single pin
Case("AnalogIn - read test", all_ports<AnaloginPort, DefaultFormFactor, analogin_test>),
};

Expected is to PASS 2 test cases. First passed, second failed.
When I run them separatelly - each of them passed.

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Oct 28, 2019

@maciejbocianski Is this similar to what you fixed in i2c - missing free function for analogin ?

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 28, 2019

Probably because the ports are already initialized when second test case is running and re-init fails. We don't have free() function in the Analogin HAL API. The driver should allow re-init I think.

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 28, 2019

@mprse Are going to add "free()" or expecting some actions from Cypress side ?

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 28, 2019

I don't have information about adding analogin free() function to HAL API. Maybe this should be considered. I suggest updating the Cypress driver to allow the reinitialization of the peripheral.

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 28, 2019

OK. I am goint to investigate this on Cypress side.

@mprse I also see similar issue with SPI. Should I creare new issue or use this one?

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Oct 28, 2019

@mprse I also see similar issue with SPI. Should I creare new issue or use this one?

new issue would work (as its for SPI)

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 28, 2019

@mprse I also see similar issue with SPI. Should I creare new issue or use this one?

new issue would work (as its for SPI)

#11757

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 28, 2019

@mprse I also see similar issue with SPI. Should I creare new issue or use this one?

This might not be a similar issue since SPI HAL API provides the free() function and the test uses it. So maybe the Cypress implementation is incorrect.

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 28, 2019

@mprse @0xc0170
I checked on Cypress side. Init() function works as expected. I called Init() 2 times, on first time it PASSED on second FAILED, it because resources are already alocated.

I also checked that AnalogIn.cpp Class doesn't have destructor. Please fix it.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 28, 2019

I think we should add free() function to HAL APIs.
Define an empty weak implementation which could be reimplemented for targets that require some actions to free the peripheral.

@0xc0170 @maciejbocianski @fkjagodzinski @kjbracey-arm @jamesbeyond what do you think?

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Oct 28, 2019

Already happening for PWMOut/Serial #10924, #11641

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

@kjbracey-arm kjbracey-arm commented Oct 29, 2019

I thought all HAL APIs had xxx_free, and it was just the case that some targets didn't implement them, because we didn't use/test them.

If a HAL API doesn't have free, like this, it should be added, and we can have a weak definition for backwards compatibility to save all updating targets. The weak thing could also cope with the SPI case.

Then a target needs to either cope with double init, ignoring the free, or handle init/free/init. (On the same pins).

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

PWMOut/Serial #10924, #11641 is a bit different case, because PWMOut/Serial provides HAL free() functions and both PRs modifies only drivers layer.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

The following peripheral APIs do not have xxx_free() functions: Analogin, GPIO, I2C.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

I will extend PR #11032 and add xxx_free() functions to HAL API, empty weak implementations and defined behavior.

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Oct 29, 2019

Same as @kjbracey-arm , also thought we already provided free.

Thanks @mprse for fixing

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

I have a question. Do you have maybe a proposition where these empty free() implementations should be added?
We already have hal\mbed_gpio.c where weak gpio_free() can be added. Should I add additional files for Analogin and I2C (hal\mbed_analogin.c, hal\mbed_i2c.c)?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

@kjbracey-arm kjbracey-arm commented Oct 29, 2019

If only to reduce compilation time, I'd be somewhat inclined to just sneak them in to AnalogIn.cpp. That covers the main real user, and drivers is always in the build?

Maybe doesn't cover some test cases though.

Otherwise, I'd go with something like hal/mbed_compat.c - that seems like it might be a good way to collect trivial default implementations.

More complex defaults (eg the pinmap stuff from #11399 in hal/explicit_pinmap.cpp) is worth separating out, but not a pile of empty free functions, I think.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

If only to reduce compilation time, I'd be somewhat inclined to just sneak them in to AnalogIn.cpp. That covers the main real user, and drivers is always in the build?

This might be a problem for HAL tests and might be misleading: HAL implementation in the drivers layer.
I like the hal/mbed_compat.c option.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

The patch can be found in PR #11032 (comment)

@yarbcy once the mentioned PR is merged you can add Cypress implementation for required xxx_free() functions and run the FPGA-Test-Shield tests again.

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Oct 29, 2019

@mprse I am not clear with proposed approach. Please provide instruction what needs to be done from my side after PR is merged.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 29, 2019

@mprse I am not clear with proposed approach. Please provide instruction what needs to be done from my side after PR is merged.

I checked on Cypress side. Init() function works as expected. I called Init() 2 times, on first time it PASSED on second FAILED, it because resources are already alocated.

@yarbcy The tests are failing because init() is called the second time on already initialized peripheral. Now I added analogin_free(), gpio_free(), i2c_free() to HAL. By default, these functions do nothing since most of the targets do not need them.
Also, tests are updated to call free() at the end of each test case to release the peripheral (so we have now sequence init/free/init/...).
Now please implement analogin_free() function and release the peripheral.

@mprse

This comment has been minimized.

Copy link
Member

@mprse mprse commented Oct 30, 2019

@ccli8 I found that you have added the following patch to i2c FPGA test shield test: 1bf6d89#diff-8369a8a5a9a2b19b1439c2e6da8d9a36

In PR #11032 I added free() functions and modified the testes. So currently the i2c test calls free() instead of gpio_set(xxx): 2ff3112#diff-8369a8a5a9a2b19b1439c2e6da8d9a36.

This might cause a regression for some Nuvoton targets and probably we will have to implement i2c_free().

@ccli8

This comment has been minimized.

Copy link
Contributor

@ccli8 ccli8 commented Oct 31, 2019

This might cause a regression for some Nuvoton targets and probably we will have to implement i2c_free().

@mprse Add implementations of HAL API i2c_free(...) and analogin_free(...) on Nuvoton targets via #11780.

@yarbcy

This comment has been minimized.

Copy link
Contributor Author

@yarbcy yarbcy commented Nov 6, 2019

@mprse Added implementation for Cypress targets. #11830

@maclobdell

This comment has been minimized.

Copy link
Contributor

@maclobdell maclobdell commented Nov 7, 2019

FYI - we are experimenting with adding priority labels to PRs based on input from partners received through partner project managers. I just added a Priority label.

@0xc0170

This comment has been minimized.

Copy link
Member

@0xc0170 0xc0170 commented Nov 7, 2019

Once #11830 is in, we can close this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.