-
Notifications
You must be signed in to change notification settings - Fork 34
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
Comments
@LMESTM , Here is my explanation for why things the way they are (for now):
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) |
@BlackstoneEngineering |
@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? |
@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? |
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. |
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) 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. |
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 .
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). |
yes makes sense. |
This is ok now - so closing ... |
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 !
The text was updated successfully, but these errors were encountered: