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

PWM test : Make PWM pin configurable #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeromecoutant
Copy link
Contributor

The new function is deducting the PinIn depending on the PinOut value.

For ex, PWM_0 is no more hardcoded connected to D2.

@BlackstoneEngineering
Copy link
Contributor

What is the point of this? The current configuration settings are in the config file mbed_app.json, this is where the config is supposed to live.

@jeromecoutant
Copy link
Contributor Author

I agree, configuration should be set in the json file.

But it is not the case...
Ex:
PWM_Period_Test< MBED_CONF_APP_PWM_0, MBED_CONF_APP_DIO_2, 10, 1000 >
=> PWM_0 is connected to D0, whatever the value of PWM_0 in json...

With the correction, test is checking the PWM_0 value to determine which digital pin is conencted

@BlackstoneEngineering
Copy link
Contributor

@0xc0170

@0xc0170
Copy link
Collaborator

0xc0170 commented Mar 21, 2017

@jeromecoutant I dont follow

This is the config:

        "PWM_0": "D3",
        "PWM_1": "D5",
        "PWM_2": "D6",
        "PWM_3": "D9",

Thus PWM_0 is D3. What else is needed that this PR fixes? PinName PWM_IN(PinName pwm_out_pin) does not contain any documentation. How are pins assigned (out to in conversion) ?

This is one of the cases there:

+        case MBED_CONF_APP_DIO_0:
+            return MBED_CONF_APP_DIO_1;

DIO0 to DIO 1 ?

@jeromecoutant
Copy link
Contributor Author

Test needs 2 pins: 1 PWM out and 1 Interrupt In.
CI-shield is connecting them.
So in the original code, PWM_0 (D3 as you said) is connected to D2, which is correct.

What happened if user is setting PWM_0=D0 in the json file?
If you want to avoid external wires, you need to set the Interrupt In to D1
This is the patch

Thx

@0xc0170
Copy link
Collaborator

0xc0170 commented Mar 29, 2017

@mray19027 your latest PR - is not resolving similar problem or can you review this one at least?

@mray190
Copy link
Contributor

mray190 commented Apr 3, 2017

PR #47 fixes this issue

The PWM test dynamically finds the paired pin connected to the PWM pin and sets the interrupt to that paired pin.
https://github.com/ARMmbed/ci-test-shield/pull/47/files#diff-8071f2cb677762e6cf5f556dac2d8a74R79

@jeromecoutant
Copy link
Contributor Author

Rebase done as PWM tests have been split, but without this PR

Each test case selects only the pwm_out_pin,
int_in_pin is no more hardcoded but defined according the selected pwm_out_pin.

@jeromecoutant
Copy link
Contributor Author

Is there any issue with that ?

@jeromecoutant
Copy link
Contributor Author

Hi all
Is ci-test-shield abandoned ?

@BlackstoneEngineering
Copy link
Contributor

@c1728p9 - care to comment here?

@jeromecoutant
Copy link
Contributor Author

CI test shield is quite important in ST automatic test setup.
This is the way we verify SPI, I2C and all others things.

@c1728p9
Copy link

c1728p9 commented Jun 7, 2019

Hi @jeromecoutant the CI test shield isn't abandoned as far as I'm aware. There is a more advanced test shield coming, but this test shield should still be supported. @donatieng does this sound right and if so who will be supporting this?

@0xc0170
Copy link
Collaborator

0xc0170 commented May 18, 2021

@jeromecoutant Shall this be recreated (rebased) or closed? It was left in air unfortunately.

@jeromecoutant
Copy link
Contributor Author

I am using this PR for each non regression tests :-)
so I can rebase it

Each test case selects only the pwm_out_pin,
int_in_pin is no more hardcoded but defined according the selected pwm_out_pin.
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

Successfully merging this pull request may close these issues.

5 participants