NRF52: implement ADC and PWM interfaces #32
Conversation
Signed-off-by: Ron Evans <ron@hybridgroup.com>
Signed-off-by: Ron Evans <ron@hybridgroup.com>
| nrf.SAADC.ENABLE = (nrf.SAADC_ENABLE_ENABLE_Disabled << nrf.SAADC_ENABLE_ENABLE_Pos) | ||
|
|
||
| if value < 0 { | ||
| value = 0 |
There was a problem hiding this comment.
Does this actually happen, and if it does, under what conditions?
| } | ||
|
|
||
| // Return 16-bit result from 12-bit value. | ||
| return uint16(value << 4) |
There was a problem hiding this comment.
Note that this would have been undefined behavior in C, as you're shifting more than the bit width (15 bits without the sign, and the value can be bigger than 15 bits after the shift). Luckily this is not C so it is well defined. That is, I haven't seen anything in the Go spec about shift overflow but I know it is well defined in TinyGo as most integers are treated as unsigned by LLVM unless it is told otherwise.
It might be slightly more obvious what is going on here when you cast first to 16 bits before shifting.
See e.g. this tweet for a related oddity in C.
| p.PSEL.OUT[0] = nrf.RegValue(pwm.Pin) | ||
| p.PSEL.OUT[1] = nrf.RegValue(pwm.Pin) | ||
| p.PSEL.OUT[2] = nrf.RegValue(pwm.Pin) | ||
| p.PSEL.OUT[3] = nrf.RegValue(pwm.Pin) |
There was a problem hiding this comment.
Is there a reason why you're setting all four channels?
Especially as you seem to be configuring only one channel below.
| // Set turns on the duty cycle for a PWM pin using the provided value. | ||
| func (pwm PWM) Set(value uint16) { | ||
| for i := 0; i < 3; i++ { | ||
| if pwmChannelPins[i] == 0xFFFFFFFF || pwmChannelPins[i] == uint32(pwm.Pin) { |
There was a problem hiding this comment.
You're picking the first available channel here? That's quite smart, although a bit magical.
I haven't tested it, but you might be able to avoid the need for the pwmChannelPins global (with the additional RAM usage) if you're directly checking pwms[i].PSEL.OUT[0]. The RAM overhead is quite small (12 bytes on a 64kB chip) so it isn't essential but it might be interesting.
Signed-off-by: Ron Evans <ron@hybridgroup.com>
|
Removed comment. What else needs to be done? |
|
I left a few review comments. Mostly I care about two questions I have about the code. |
|
Please take a look at my inline responses, and see what you think. Thanks. |
|
It seems like they don't show up at my side - I see just 2 inline comments from you. |
|
Seems like the comment threads are a bit too threaded in this PR for Github to handle? Anyhow, the 2 things were I think:
I did verify that the jitter of an analog device can bring this value to just below the zero reference when used with VDD as the supply voltage, causing the value to overflow when converted to unsigned. I used my rotary dial to generate the resistance, plugged directly into VDD, GND, and P0.03 (A0) and when turned down to the lowest resistance, the value returned matched the behavior accounted for by the conditional.
I figured we could look into adding that in a future PR to TinyGo, hence why I submitted the PR you are looking at right now. |
|
Strange, I haven't seen such things on GitHub before...
I didn't expect the hardware to do that, but OK.
Hence the globals? Yeah, that's not a big deal. I was more referring to this one, machine_nrf52.go:139:
|
|
I borrowed the implementation from https://github.com/sandeepmistry/arduino-nRF5/blob/master/cores/nRF5/wiring_analog_nRF52.c#L229-L232 without getting too far down the rabbit hole. This is the same code as is in https://github.com/adafruit/Adafruit_nRF52_Arduino The code from sandeepmistry/arduino-nRF5#195 looks better as a second pass thru, but I figured to look into that as a future enhancement. |
|
I understand. Maybe not too clean but overall this PR looks great so I merged it in 8f7b7e6. |
This PR adds the ADC and PWM interfaces for the nrf52/pca10040.