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

Why running the same test on 4 PWMs ? #26

Closed
LMESTM opened this issue Jan 20, 2017 · 10 comments
Closed

Why running the same test on 4 PWMs ? #26

LMESTM opened this issue Jan 20, 2017 · 10 comments

Comments

@LMESTM
Copy link
Contributor

LMESTM commented Jan 20, 2017

The PWM API tests run the same test over 4 different PWMs that are expected to be available on
"PWM_0": "D3",
"PWM_1": "D5",
"PWM_2": "D6",
"PWM_3": "D9",
as defined in mbed_app.json

This makes a very strong constraint on the board design and all the boards don't comply with it. Also the tests use PWM_X as the PWM output as defined in mbed_app.json, but the interrupt in input that is used to verify PWM behavior is hard coded in the TESTS/API/Pwm/Pwm.cpp file, as MBED_CONF_APP_DIO_2, 4, 7 and 8 respectively.

Would that be OK to only consider 1 PWM output and respective input rather than 4 and also use a generic entry for the interrupt in input, like "MBED_CONF_APP_PWMIN_0" for instance ?

thanks !

@LMESTM LMESTM changed the title Why running the same test on 4 PWMs Why running the same test on 4 PWMs ? Jan 20, 2017
@BlackstoneEngineering
Copy link
Contributor

@LMESTM ,

Here is my explanation for why things the way they are (for now):

  1. The software was written to test boards that are compliant with the Arduino R3 header hardware spec, that is the spec mbed recommends to its partners and the one we see the most used. That said we are aware that usually most vendors do not implement the full spec, so we are working on a possible work around (see Speed Up Tests #3 below)
  2. If a board has more than 1 PWM is it necessary to check all of them, I have seen multiple boards pass with PWM0 working but PWM 2-4 do not work because the software port was botched. Testing all the PWM's (if they are available) should be done to prevent this. We wrote these tests into the CI Test shield because it is necessary to test everything possible and I would prefer to automate it all than have to do it by hand.
  3. We are aware that some boards may not have PWM2-4, or some subset there in. I am working with the partnership team on a solution. For the moment, if your board does not support PWM2-4 then the failure is to be expected. Simply tell your partner manager this and make sure your pinout diagram reflects this an you will be fine. I know its not an optimal solution, and we're working on a better way.

As to the suggestion for adding PWMIN_0-4 defines to the mbed_app.json file, I will leave that decision to @0xc0170 as he is the maintainer. The intial draft was done with the pins hard coded to avoid clutter in mbed_app.json file and because the CI Test shield has wire traces that loopback to those pins (again, per the Arduino R3 header spec). So adding another perameter to the mbed_app.json file would not necessarily fix things, because if you wanted to change where the pins looped back you would need to physically modify the CI Test shield. If you have to modify the shield to pass your tests then you have to supply us with 10x of those custom boards and submit the software in a seperate branch so we can add it to our CI test farm. (We do not allow fly wires on the CI Test Farm as things break too easily)

@LMESTM
Copy link
Contributor Author

LMESTM commented Jan 23, 2017

@BlackstoneEngineering
thanks for you answer. I understand the background.
Reducing the number of PWMs to tests and adding defines in mbed_app.json file, would also allow various work-arounds, like picking up 1 or 2 available PWMs out of the 4 possibilities on ci-test-shields.
Cheers
Laurent

@BlackstoneEngineering
Copy link
Contributor

@0xc0170 we should add this to the list of things to consider in the next cycle. Either adding configuration to the json file or making 'levels' of compliance, where the top level tests everything.

@LMESTM just to confirm, your use case is that only say PWM1 and 2 are hooked up, and not PWM 3 or 4, so you're getting failures for PWM 3/4 when your board doesnt support them, correct?

@LMESTM
Copy link
Contributor Author

LMESTM commented Jan 24, 2017

@BlackstoneEngineering yes indeed. For instance https://developer.mbed.org/platforms/ST-Nucleo-F410RB/ has PWMs on D2, D7 and D8, but not on other [D2 - D9] pins

@0xc0170
Copy link
Collaborator

0xc0170 commented Feb 2, 2017

@BlackstoneEngineering yes indeed. For instance https://developer.mbed.org/platforms/ST-Nucleo-F410RB/ has PWMs on D2, D7 and D8, but not on other [D2 - D9] pins

@LMESTM Can you send a patch with a proposal to have pins configurable per target? Or propose it here how ti would look?

@BlackstoneEngineering
Copy link
Contributor

All, please go take a look at https://gist.github.com/BlackstoneEngineering/e3db036272daa2e5424076fbca8b9d43 . I feel it may be more productive to collaborate on the redesign for more modularity than try to patch it in here.

@LMESTM
Copy link
Contributor Author

LMESTM commented Feb 3, 2017

@LMESTM Can you send a patch with a proposal to have pins configurable per target? Or propose it here how it would look?

The principle is very basic. Mainly define the digital input for checking pwm in mbed_app.json file e.g. PWMIN_0, althgouh I'm not so happy with this misleading name)
"PWM_0": "D2",
"PWMIN_0": "D3",
"PWM_1": "D4",
"PWMIN_1": "D4",
"PWM_2": "D5",
"PWMIN_2": "D6",
"PWM_3": "D7",
"PWMIN_3": "D8",

Then the PWM.cpp would rely on it.

In case a platform does not have full Arduino compatibility, it would be able to redefine any of the pins to match the HW in mbed_app.json.

@0xc0170
Copy link
Collaborator

0xc0170 commented Feb 3, 2017

I feel it may be more productive to collaborate on the redesign for more modularity than try to patch it in here.

Agree. That led me to an idea, it might be better to use git repo than gist (for review). If we can send this to this repo as PR (RFC type, docs folder for instance, once we all agree on the form, it could become doc in this repository), and start discussing it here, see how the docs is changing .

The principle is very basic. Mainly define the digital input for checking pwm in mbed_app.json file e.g. PWMIN_0, althgouh I'm not so happy with this misleading name)

Thanks, I had something similar on mind. Would it make sense to have one test for arduino compability (that one would fail in this case as not all pins are PWM capable that should be) but PWM test would not fail as it would take input from config file as you proposed above (=PWM is working as it was tested on some pins, but not full set in some cases). Then if I read the report. I would see this platform does not conform to some assumptions about pinout, but the peripheral driver is all good. This would not result in misleading results (red test just because one pin is not wired, etc).

@LMESTM
Copy link
Contributor Author

LMESTM commented Feb 3, 2017

Thanks, I had something similar on mind. Would it make sense to have one test for arduino compability

yes makes sense.
Note that I don't see where Arduino mandates that all D2-D9 pins can support HW PWMs.
In Arduino, PWMs are often considered as the output of a call to "analogWrite(pin, dutyCycle),". Basically Arduino "PWMs" also include pseudo analog emulation, or even emulated PWMs with GPIOs, not HW timers based PWMs. In MBED, as I see it, PwmOut are HW PWMs, unless we implement SW PWM in MBED ...

@LMESTM
Copy link
Contributor Author

LMESTM commented Jun 21, 2017

This is ok now - so closing ...

@LMESTM LMESTM closed this as completed Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants