Opt-in PWM conflict avoidance

[henrygab]

Fix for #459, #525

Creating as a draft PR so @hathach can review the work-in-progress.

• HardwarePWM object tracks an opaque uintptr_t token for ownership (take and release); Note that this is voluntary, and no enforcement is provided ... it just simplifies detection of use by code updated to use these APIs
• analogWrite() opts-in to ownership checks
• analogWriteResolution() only immediately affects PWM peripherals "owned" by analogWrite
• analogWriteResolution() value is cached, and applied when analogWrite() takes ownership of a new PWM peripheral
• servo() opts-in to ownership checks
• tone() opts-in to ownership checks

Validation (tested) that:

• takeOwnership refuses to acquire PWM used by legacy code (pin in use)
• takeOwnership refuses to acquire PWM owned by other code (token non-nullptr)
• takeOwnership successfully acquired
• releaseOwnership works
• analogWrite() opts-in to ownership checks
• analogWriteResolution() only immediately affects PWM peripherals "owned" by analogWrite
• analogWriteResolution() value is cached and applied when analogWrite() takes ownership of a new PWM peripheral
• servo() testing complete
• tone() works with time-limited playback
• tone() releases PWM when done playback
• tone() works with unlimited playback
• tone() stops existing tones, plays new tone
• analogWrite() releases PWM when pin is no longer used for analogWrite() -- deferring this fix to another PR, as requested by @hathach

[henrygab]

All changes made. I've written a unit test to validate all affected functionality.

FYI ... I've not validated the above changes using that unit test yet. Bugs may still exist, but the core concepts (including your excellent earlier feedback on a void* / uintptr_t token) should now be in there.

[henrygab]

Well, nothing crashes using a stress-test sketch.

Stepped through analogWrite() calls, to ensure arbitration was working, and fixed inverted logic checking for connected pins.

Scope shows various pin outputs changing in stress test, so at least some functions are working as expected. Functionality is not validated, but it's at least looking positive thus far.

[henrygab]

@hathach -- I do not have any servos available; Are you able to do quick functional validation for this PR?

All other functionality has been validated by the functional test and the stress test sketches written for this purpose.

Accordingly, I am moving this PR from WIP to ready for your review.

[hathach]

Thanks @henrygab I will check it out asap. Normally I would tell you to order from adafruit for servo and/or other goodies for free as well. Though It is a tough time 😅😅

[henrygab]

Hi @hathach,

Here's a summary of servo changes:

• a3517bf -- Fix error paths related to attempts to use too many servos
• a3517bf -- Acquire the opt-in ownership on attachPin()
• 7a5a432 -- when detach() from last channel, stop the PWM peripheral and release ownership

Note that the return value from attach() that represents an error was updated. This is because a value of zero is a VALID channel index. Instead, it now returns the newly-defined INVALID_SERVO (0xFF).

[hathach]

will test it soon, test it soon ..... 😅

[henrygab]

Keeping the thread current.

[hathach]

thanks @henrygab for gentle reminder, I don't forget it, have tried to see if I could review it quickly, but it needs a bit of time for reviewing (just got myself too busy with other works). I will definitely review it next week. Thank you very much for your patient as always :)

[henrygab]

Keeping this thread current.

[henrygab]

Every two weeks or so, I will add another comment.

[hathach]

Thank you @henrygab for putting lots of effort into this PR. It is a bit overwhelming for me to review. Overall it is a great PR with an excellent approach, though I find it trying to solve many issues at once, e.g the race condition in the tone() should be on separated PR. That would make it easier to review and merge. There is too many modification that I could only barely review it this time. Will try to spend more time to review later on after pr is updated. Thank you for your patient so far.

PS: I think you should remove most of the LOG_LV3(), too much normal information, mostly we only want to log what user find interesting.

PS: I think the find & add channel to PWM should be a method of HardwarePWM() that will help to reduce this portion of code across the module (anlog/tone/servo).

[henrygab]

@hathach -- Can you look at why the builds are all failing with the following error?

| Bluefruit52Lib       | StandardFirmataBLE.ino              | failed    |  5.21s |
In file included from /home/runner/work/Adafruit_nRF52_Arduino/Adafruit_nRF52_Arduino/libraries/Bluefruit52Lib/examples/Peripheral/StandardFirmataBLE/StandardFirmataBLE.ino:31:
/home/runner/Arduino/libraries/Firmata/Firmata.h:131:17: error: friend declaration of 'void encodeByteStream(size_t, uint8_t*, size_t) const' specifies default arguments and isn't a definition [-fpermissive]
  131 |     friend void FirmataMarshaller::encodeByteStream (size_t bytec, uint8_t * bytev, size_t max_bytes = 0) const;
      |                 ^~~~~~~~~~~~~~~~~
Error during build: exit status 1

All other examples build fine ... I think this problem exists in the main branch, and is not caused by this PR.

Let me know?

[hathach]

@henrygab the ci build error is due to the gcc v9 with existing firmata 2.5.8 build
#527 (comment)

I bumped up the firmata to use master but it got into another issue that need to make an PR to firmata repo to fix. I will do it later on. Please just skip that example for now.

[henrygab]

@hathach -- I cannot change what the CI build does, and it will remain marked as failing. However, I've addressed the majority of issues found, and believe this to be ready for your approval now.

[hathach]

@hathach -- I cannot change what the CI build does, and it will remain marked as failing. However, I've addressed the majority of issues found, and believe this to be ready for your approval now.

Indeed, it is not your issue, I just submitted the PR to firmata to fix the build issue with it, hopefull that got merged soon enough firmata/arduino#459

[hathach]

Excellent PR as always, except the change to use freeRTOS mutex to achieve actual mutex, other review are optional. Some could be simplified but it is not required. For mutex, I have no idea what the builtin atomic function does, and how it can prevent the preemptive kernel of freeRTOS. The really only way compiler could do that is completely disable interrupt bit on cpse of ARM Cortex, which can cause issue with BLE timing. The more proper way is using freeRTOS mutex, but in this case we could just use the

• https://www.freertos.org/taskENTER_CRITICAL_taskEXIT_CRITICAL.html
• https://www.freertos.org/taskENTER_CRITICAL_FROM_ISR_taskEXIT_CRITICAL_FROM_ISR.html

which effectively disable freeRTOS scheduler without affecting the BLE interrupt.

[henrygab]

FYI -- still on my radar, but real life delaying....

[hathach]

FYI -- still on my radar, but real life delaying....

No worry at all, don't be bothered with this, your real life problem is 💯 times more important

[henrygab]

OK, rebased the already-reviewed items.

New commits start at commit 29843da, which is a significant rewrite of tone().

click to expand ...

Given the prior API's restricting frequency to range [20 .. 25000], the total number of pulses that could be requested if duration was maximum value fits within 37 bits.

The nRF52 PWM peripheral has three ways to extend the number of samples when playing a long loop:

1. Loop count ... well known, 16-bits in register
2. sequence having multiple samples (SEQ[N].CNT) ... well-known, but takes memory
3. refresh ... less well known, 24-bits in register

Tone() doesn't do anything fancy ... all the sequence duty cycles are the same value. Therefore, could use loop count and refresh values to provide up to 2^40 pulses as a single loop ... which is three bits more than tone() requires!

---

Tested on Feather nrf52840 Express. Validated both frequency and count of pulses (manually... up to 20 pulses) using RIGOL DS1054 scope. Validated approximate timing for tone, up to ~30 seconds.

Now that it's possible to do so (no ISR), should tone() use any other available PWM peripheral, if PWM2 is not available?
(Note: would still try to use PWM2 first, for compatibility....)

[henrygab]

@hathach - ready for you to test servo. you also wanted to test tone() fix.

[hathach]

@henrygab thank you very much, too many changes, that is lots of thought. I will try to review as soon as I could.

[henrygab]

@hathach -- Please wait on merge... I am seeing an odd behavior during a new test, and need to investigate.

[henrygab]

@hathach -- OK, found the regression, and re-ran the full battery of tests with 'scope attached. Looks good now.

[hathach]

tone introduces too many changes. It is a great PR as usual. I appreciate your time and effort spending on the PR. You did give out lots of thought for the feature. I have only done a surface scratching review, since the changes is too many (haven't looked at the actual core function yet). I guess there may be a few more round of reviews before this can be merged.

[henrygab]

All comments have been addressed.

[hathach]

This PR has been in progress for too long, it is my fault for not spending enough time for this ( busy with other project). There is also a difference in our point of view in some certain situation, which is a good thing (why need a duplicate view :D ). However, It is indeed not really nice to ask you to modify code repeatedly especially without conform. So yeah, this got merged now, I will do a "clean up" PR later on and tag you as reviewer later on. Thank you very much for your patient and really hard working on this PR.

[hathach]

please remove the lamda function, it is over-complicated, also don't use NRFX_ASSERT() use the arduino VERIFY() instead. Also freeRTOS's mutex, unlike semaphore, you don't have to give before taking mutex. Just create and it is ready to be taken.

PS: with the new implementation of tone() there is no ISR invovled, what is the mutex protect, sorry for the question, I am kind of forget.

[henrygab]

@hathach ... As to why I left this in, it was to allow for enabling the use of any available PWM module in a later checkin. As there was no interest in changing tone() to remove the requirement to use PWM2, the next PR will include this, no problems.

[hathach]

I still prefer using freeRTOS mutex, since I have no idea how this atomic function is implemented in ARM arch with preempted RTOS with/without ISR.

[henrygab]

Previously, you were uncomfortable because the CAS operation was using a GNU C++ macro, which you said was "legacy, mainly targeted Intel CPU".

I definitely looked at rewriting using a mutex. But, if you try, you will see that it quickly gets much more complicated, especially when you have to consider order of initialization for statics, error paths, etc.

The good news is that C++ has defined the atomic operation. Therefore, since it's ensured to work per the C++ standard, I'm not understanding your concern about ARM or whether preemption is enabled in freeRTOS?

From what I could see, the use of the C++ atomic fully addressed your concerns about "legacy" and "mainly targeted Intel CPU".

[hathach]

I am new to these atomic operation, unless somebody could point out how these are implemented on ARM, I am not convinced. I suspected it would enable/disable with cpsie/cpsid which is not as ideal as using RTOS critical API. Anyway, I will do the change myself later on.

[henrygab]

... It is indeed not really nice to ask you to modify code repeatedly especially without conform.

I apologize for the frustration. I believe there was only one item that was unchanged (without your saying it's OK). This was the use of floating point, and my last response stands. Because the FP code had serious potential problems, and because I could not attest to its working correctly, I would not attach my name to a commit that added those back in ... it would not meet my quality standards.

At the same time, I attempted to be extremely flexible, including when your requests could been seen as unreasonable:

• renaming function-local variables
• removal of static assertions (inc. when you said they are "always true")
• removal of comments (many, many ...)
• whether an empty function is defined inline in header, or defined in the cpp file
• removal of throw() / noexcept ... because you thought they indicated that the functions would throw (?!)

Importantly, thanks to your code review and our discussions, I believe the PR was improved:

• got rid of the interrupt handler altogether, and tone() is processed entirely in caller's context
• tone() output is now entirely handled by the hardware peripheral (zero-cpu, immune to SD interrupts)
• much lower cyclomatic complexity in tone()
• code's conformance to class / enum / macro casing

So, I thank you for the work and help getting this PR to an acceptable state.

[hathach]

I apologize for the frustration. I believe there was only one item that was unchanged (without your saying it's OK). This was the use of floating point, and my last response stands. Because the FP code had serious potential problems, and because I could not attest to its working correctly, I would not attach my name to a commit that added those back in ... it would not meet my quality standards.

You have nothing to apologize for, you have done a great PR, but I wouldn't want you to spend too much time on this. I am really appreciated your effort and willingness to work on this :)