-
Notifications
You must be signed in to change notification settings - Fork 494
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
Add helper functions to HwPWM Instance #760
Conversation
This allows for PWM configuration when direct register access is required.
Given that no function exists to enumerate added pins, this allows for the HwPWM instance to be completely reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeAllPins
is OK however baseAddr()
is not accepted, since that could cause incosistent between private value of HwPWM object when configured externally.
@hathach, It seems to me it would be nice to allow a programmer to takeOwnership of a HwPWM to use it for some purpose , then return it back to the available pool when done. Without this, how would one take ownership of a HwPWM and use it manually if HwPWM does not expose the actual NRF_PWM baseAddr? To do this today, my code needs to make an assumption that the following mapping exists:
and then maintain a separate hardcoded private mapping such that, when I take ownership of HwPWMx, I use the corresponding NRF_PWMx. However, this is a BIG assumption and could easily go wrong if this mapping were to change between boards or a future revision of the Aradfruit_nRF52_Arduino core. It would be much more robust if, after takeOwnership of the HwPWMx I could just ask it what NRF_PWM to use for direct configuration. Yes it is true that you could shoot yourself in the foot, but to be honest, you can already do that if you takeOwnership of a HwPWM, then 'addPin', and then releaseOwnership. Then next process to 'takeOwnership' will then be unaware that a pin mapping exists and have inconsistent behavior. In fact, prior to this merge request and the removeAllPins() function, you would have to add all the pins and then remove all of the pins in order to ensure your HwPWM was properly reset back to a know good state. Additionally, you could just use the NRF_PWMx interface directly and poof, inconsistent behavior. IMO, preventing programmers from accessing the value of _pwm via baseAddr is an artificial constraint that does not provide any guarantees of consistency in the HwPWM interface. Whether you use the baseAddr() method for nefarious purposes or use NRF_PWMx directly there is nothing that the Adafruit_nRF52_Arduino core could do to prevent inconsistencies that would arise. If we allow this addition, we open the door for a whole host of new creative uses of the HwPWM, much more inline with Nordic's intentions for the flexibility of the PWM peripheral. Look at Tone.cpp in this repo, this is a great example:
Instead of dynamically selecting a HwPWM from the pool, the programmer must hard code a preferred HwPWM2 so it could be access directly with a static mapping on Line 47 to NRF_PWM2. This code would be so much cleaner and less tightly coupled if HwPWM exposed baseAddr(). As time goes on, we should work to ensure that HwPWM does properly reset its NRF_PWM and pin configuration to a know good state, so we CAN offer a guarantee of consistency to the next consumer to takeOwnership of a HwPWM. Does that make sense or would you still suggest that I remove baseAddr()? |
@hathach Does the above make sense or would you still suggest that I remove baseAddr()? |
baseAddr() still needs to be removed. All changes need be done via class member function. You don't need to use this Hardware PWM though, just use the bare NRF_PWM if you prefere to manage your own PWM instance. |
@hathach Thank you for considering the addition. I will remove the baseAddr() function. FWIW, I DO still have to use this HardwarePWM, so that it is reserved and removed from available use. That also means I have to maintain the dangerous parallel mapping described above or a perpetual fork of this repository. If you have a better idea on how to ensure that future releases of the BSP do not break that parallel mapping, please let me know. On a side note: Tone.cpp (in this BSP) currently violates the rule about causing "inconsistent between private value of HwPWM object when configured externally". It directly manipulates NRF_PWM2 in a way that cannot be detected (or reset) by the HwPWM interface (e.g. setting NRF_PWM2->SHORTS). Once the Tone class is used (and it releases ownership of HwPWM[2]), HwPWM[2] cannot be used again for anything else without risk. If I have some free time, I will try and submit a pull request to resolve as many of those that I can find. |
Tone.cpp is written by others and I am too lazy to wrap it up with HwPWM. I may have a second thought on getBase() however, I currently don't have time to revise this at the moment. Maybe later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look good, thank you for your PR
These functions allow for full control of the PWM configuration when direct register access is required and supplies a necessary function to reset the HwPWM to its default configuration.
A typical workflow might be:
takeOwnership
of an available HwPWM instance through the standard APIbaseAddr
of that HwPWM instance if direct configuration using NRF_PWMx is requiredaddPin
(s) as necessary to the HwPWMenable
the HwPWMstop
the HwPWM when finishedremoveAllPins
from the HwPWMreleaseOwnership
and return the HwPWM instance back to the available pool