search

mbed-ce / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
37 stars 12 forks source link

Fix PwmOut::resume() for static pinmap usage #281

Closed multiplemonomials closed 2 weeks ago

multiplemonomials commented 1 month ago

Summary of changes

When running CI shield tests against master, I noticed that switching the LPC1768 to static pinmaps broke the PWM test. And it was a weird error, too -- an assertion failure with "pinmap not found for peripheral". As it turns out, this PR which added static pinmaps to PwmOut made a big mistake -- they didn't notice that PwmOut::resume() calls init() a second time, and this version of init() is hardcoded to use a regular PinName, not a static pinmap. If you had constructed PwmOut with a static pinmap, the _pin member variable never gets initialized, so it tries to map an undefined pin for PWM output. Bam, (likely) assertion failure.

This PR fixes the issue by storing the pinmap and the init function to use as member variables, and then using the correct one later on. I tried to copy how this is done in the SPI class, where they have the same sort of issue.

Impact of changes

PwmOut::resume() no longer causes assertion failure when using a static pinmap. Also, PwmOut now locks the deep sleep correctly when using a static pinmap (this was missed in the original code)

Migration actions required

Documentation

None

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Now passes CI shield PWM test!

Reviewers

JohnK1987 commented 1 month ago

Did you test this change also with another targets (Not just LPC1768)?

multiplemonomials commented 1 month ago

I can test on another target like STM32U5, just, hardly any targets support static pinmaps, so I probably won't be able to test on another static pinmap target

JohnK1987 commented 1 month ago

STM32F249ZI has implemented static pin map but I suppose you do not have Nucleo-F429, right? I do not understand it very well but my concern is that when i look to implementation of this feature. It looks differently in implementation on target level.

STM32:

LPC1768:

Probably I am wrong, but I rather ask.

multiplemonomials commented 4 weeks ago

Good point, I do have a NUCLEO_F429ZI! I tested and the PWM test didn't work, but for reasons unrelated to static pinmaps. After #283, it works perfectly even with this PR merged.

multiplemonomials commented 4 weeks ago

RE your comment, I think that the init direct function is allowed to be static iff static pinmaps are not enabled, which is what those ifdefs are doing. I suppose that static on the LPC1768 pinmaps might be a mistake, it doesn't look like it has to be there according to the docs.

multiplemonomials commented 2 weeks ago

@JohnK1987 this look good to approve? I fixed the "static" in the LPC1768 pinmaps

multiplemonomials commented 2 weeks ago

Actually hold on, the static keywords do need to be there. This prevents multiple definition issues if multiple things include PeripheralPinMap.h. STMicro is doing something a bit different, where they declare the pinmaps as extern in another header and have all the C sources include that. But I think that the simpler way is to use static.

multiplemonomials commented 2 weeks ago

@JohnK1987 good to approve?

JohnK1987 commented 2 weeks ago

I missed the change was already done.