P2 PWM fixture tests. PWM HAL fixes

[scott-brust]

Problem

P2 cannot read a PWM pin itself using pulse_in(). This PR adds a PWM fixture test that uses one pin to generate a PWM output and another to measure it via pulse_in().

Solution

Port the no_fixture_long_running/pwm tests into a wiring/pwm fixture test app.

• In general, tests configure one pin as the "output" pin, which a PWM signal is generated on. Each other PWM pin is configured as in "input" pin, and pulse_in is used to measure the output pins signal. Each permutation of output/input pins is configured and run for each test.
• Capacitance doesnt seem to be an issue but if it is, the test can be modified to only test certain pairs/combinations of pins

Implementing these tests resulted in some PWM hal fixes/changes:

• Default PWM resolution changed to "8 bit" resolution (The KM4 PWM timer Auto Reload and Count registers are always 16 bit values, and vary depending on the prescaler value as well. But when the PWM resolution is changed via hal_pwm_set_resolution() the register values are calculated to match the new "resolution".)
• pwmInfo state structure refactored a bit. Each channel has its own state to make pwm sleep/wake easier
• PWM Frequency calculations improved by use of prescaler
• GPIO HAL / pinMode() will disable PWM configuration from pin.
• pwmPinDeinit() will not clear pin from pwmInfo state, allowing hal_pwm_sleep() to work as intended

Steps to Test

• To build:
  make PLATFORM=p2 TEST=wiring/pwm
• To setup: connect all P2 PWM pins together: D1, A2, A5, S0, S1
• To run: open USB serial connection, hit t to start all tests

Example App

use wiring/pwm

Test output (verified with logic analyer)

Running tests
Test PWM_01_LowDCAnalogWriteOnPinResultsInCorrectPulseWidth passed.
Test PWM_02_LowFrequencyAnalogWriteOnPinResultsInCorrectPulseWidth passed.
Test PWM_03_HighFrequencyAnalogWriteOnPinResultsInCorrectPulseWidth passed.
Test PWM_04_PwmSleep passed.
Test PWM_05_CompherensiveResolutionFrequency passed.
Test summary: 5 passed, 0 failed, and 0 skipped, out of 5 test(s).

References

---

Completeness

• User is totes amazing for contributing!
• Contributor has signed CLA (Info here)
• Problem and Solution clearly stated
• Run unit/integration/application tests on device
• Added documentation
• Added to CHANGELOG.md after merging (add links to docs and issues)

[scott-brust]

When a new resolution is set, should we recalculate the ARR and CCMRX register values (ie duty cycle) to update the new values?

[technobly]

If a pin is already running a non-zero PWM output, and the resolution is changed... we don't have docs that say what is expected to happen. I don't think the duty cycle value should be re-adjusted though (that's up to the user), but the frequency probably should remain the same (if it already doesn't). We probably should have a note that says something like, "It's recommended to set the resolution before calculating the duty cycle value and setting the PWM to a non-zero value." ?

[scott-brust]

We will store the new resolution and duty cycle value in the PWM HAL, but it wont be applied to the timer registers and the PWM output duty cycle / frequency wont change.
The user will have to call analogWrite() to get their duty cycle/frequency changes to take effect. So there is no new behavior here, but yeah could be documented better?

[avtolstoy]

We will store the new resolution and duty cycle value in the PWM HAL, but it wont be applied to the timer registers and the PWM output duty cycle / frequency wont change.
The user will have to call analogWrite() to get their duty cycle/frequency changes to take effect. So there is no new behavior here, but yeah could be documented better?

That's the expected behavior.

[scott-brust]

Setting the pin to PIN_INVALID made it impossible to re-generate the PWM signal on the suspended PWM channel when hal_pwm_sleep(true) / hal_pwm_sleep(false) was called. Thats why I changed pwm hal to track each channel state. When a pin is init'd, the pin number will always be preserved

[XuGuohui]

Any reason why we need to change the algorithm? The loop will take longer to figure out the desired ARR andd prescaler.

[technobly]

I think it was because of a bug found with lower resolutions. As a compromise can we pre-calculate things, and then drop into the while() loop to make a quick tweak, instead of starting from calculated_prescaler = 0 every time? I'm sure this is pretty fast already, and also I guess this does get called on every call to analogWrite* so the time it takes could be important to some.

[scott-brust]

When the calculated arr value was larger than 16 bits, it was truncated at 16 bits.
Then the truncated value was used to calculate the prescaler value. This would lead to incorrect PWM frequencies, especially at low frequencies.

For example, the default PWM frequency is 500hz. that means arr is calculated to be:
40mhz / 500 hz = 80000
which is greater than 1<<16 -1 (ie 0xFFFF) and is truncated to 0xFFFF

When using the truncated value to calculate prescaler:
40mhz / 500 hz / 0xFFFF = 1.22
1.22 + 0.5 = 1.72
Casting to uint8_t rounds to 1. Subtracting 1 from that means the prescaler should be 0.

arr set to 0xFFFF and prescaler 0 gives a frequency calculation of:
40mhz / 0xFFFF = 610 hz
Which is not the desired 500hz output.

This method is somewhat slower but should be more accurate.

[XuGuohui]

That makes perfect sense. 👍

[technobly]

Can we maybe cache the frequency value and only re-calculate if the frequency for the pin changes? This seems like something that only needs to be calculated once when the frequency is changed. Usually you setup a PWM to be a certain resolution/frequency and then only vary the duty cycle after that.

[scott-brust]

Determining the prescaler is only done when changing the frequency. Do we think this will be done frequently enough to need to improve performance here?

Modifying duty cycle via hal_pwm_update_duty_cycle_ext() only updates the CCR register, which should be fast.

I would prefer to not make this more complicated unless needed

[technobly]

It looked to me that calculateArrAndPrescaler was called from every call to hal_pwm_write_with_frequency_ext which also gets called by hal_pwm_write*.

[technobly]

Tests passed several times for me 👍🏼
After the 3rd test run I noticed this failure:

Assertion failed: (avgPulseHigh=342) <= (316=316), file tests/wiring/pwm/pwm.cpp, line 107.
Test PWM_01_LowDCAnalogWriteOnPinResultsInCorrectPulseWidth failed.

So I ran it 100 more times in a loop, only failed 3 times:

Assertion failed: (avgPulseHigh=283) <= (250=250), file tests/wiring/pwm/pwm.cpp, line 119.
Assertion failed: (avgPulseHigh=278) <= (250=250), file tests/wiring/pwm/pwm.cpp, line 119.
Assertion failed: (avgPulseHigh=300) <= (250=250), file tests/wiring/pwm/pwm.cpp, line 119.

Would it be ok to increase the tolerance here and just check for really blatant errors?

[technobly]

I think it was because of a bug found with lower resolutions. As a compromise can we pre-calculate things, and then drop into the while() loop to make a quick tweak, instead of starting from calculated_prescaler = 0 every time? I'm sure this is pretty fast already, and also I guess this does get called on every call to analogWrite* so the time it takes could be important to some.

[scott-brust]

Would it be ok to increase the tolerance here and just check for really blatant errors?

Since this test is manual I think its probably fine to leave as is and just accept that a tight tolerance means some false failures? I dont really know how the margins of error were originally determined anyways.