Fix bug #169 - analogWrite #195

micooke · 2017-09-29T04:37:03Z

wiring_analog_* : fallback to digitalWrite if no available PWM channel (copied from AVR core)
wiring_analog_nRF52.c : convert pwmChannelPins & pwmChannelSequence -> pwmContext to semi-standardise pwm pin allocation and pwm status between nRF51 and nRF52
wiring_private.h : Move pwm structures defines out of wiring_analog_* into wiring_private and make the instantiation of these structure externs instead of statics
wiring_digital.c : disable the appropriate pwm timer when a digitalWrite is sent to an allocated (from analogWrite) pwm pin, and free up the pwm channel for re-allocation

wiring_analog_* : fallback to digitalWrite if no available PWM channel (copied from AVR core) wiring_analog_nRF52.c : convert pwmChannelPins & pwmChannelSequence -> pwmContext to semi-standardise pwm pin allocation and pwm status between nRF51 and nRF52 wiring_private.h : Move pwm structures defines out of wiring_analog_* into wiring_private and make the instantiation of these structure externs instead of statics wiring_digital.c : disable the appropriate pwm timer when a digitalWrite is sent to an allocated (from analogWrite) pwm pin, and free up the pwm channel for re-allocation

micooke · 2017-09-29T04:37:41Z

Tidy-up of #193

sandeepmistry

@micooke Thanks for submitting!

sandeepmistry · 2017-10-02T00:53:15Z

cores/nRF5/wiring_analog_nRF51.c

    }
  }
+
+  // fallback to digitalWrite if no available PWM channel
+  if (ulValue < 128)


Can the write resolution be higher than 8-bits? If so, we need to adjust this.

Yes, and on it!

One thing i noticed is that the read and write resolution isnt bounded and for the NRF52 they are stored as signed integers so you could set the read or write resolution to +/-2e9!

If its alright ill add a static int with the half max value and a switch statement to set it in writeResolution so that valid values are used (and so they arent re-calculated on each analogWrite)

For reference;

nRF51 TIMER1 (the only timer currently used for PWM) is either 8b or 16b write resolution

nRF52 is up to 32b write resolution for all 12 PWM channels

sandeepmistry · 2017-10-02T00:55:19Z

cores/nRF5/wiring_digital.c

@@ -86,18 +93,42 @@ void digitalWrite( uint32_t ulPin, uint32_t ulVal )

  ulPin = g_ADigitalPinMap[ulPin];

+  for (uint8_t i = 0; i < PWM_COUNT; i++)


What do you think about moving this to a private analogWriteOff (or something with a better name) function in cores/nRF5/wiring_analog_nRF5x.c?

Rather than creating another function I could change analogWrite so that it turns off PWM when a duty cycle of zero is passed?

Its up to what you prefer as you are the owner and this affects the API. Either implementation is easy to implement.

micooke · 2017-10-06T02:09:41Z

Ok, can of worms officially opened 💥
nrf51 is done, but i noticed something i should have noticed earlier - the nRF52 only uses 3 out of 12 PWM channels. It has 3 PWM/TIMERS with 4 compare channels each, and in the core each compare channel output pin points to the same pin - so 3 outputs max (currently). This will need some work to fix it up, and luckily now i have the hardware to test it on!

micooke · 2017-10-06T02:10:43Z

Sorry i should have asked, @sandeepmistry , @dlabun - do you want me to fix the nRF52 PWM channel issue here or in a separate PR?

The two issues are somewhat linked in that to enable the extra pwm i need to add another parameter in the PWMContext structure, which will then require me to update how the TIMER/PWM modules are turned off.

dlabun · 2017-10-06T02:42:04Z

Sure, go for it.

micooke · 2017-10-06T02:53:13Z

cores/nRF5/wiring_analog_nRF51.c

+  }
+  else
+  {
+    digitalWrite(ulPin, LOW);


Should be HIGH - copy/paste error

micooke · 2017-10-06T02:53:49Z

cores/nRF5/wiring_analog_nRF52.c

+  }
+  else
+  {
+    digitalWrite(ulPin, LOW);


The same copy/paste error again

…s with M channels each) * replace static 8b resolution for nRF51 and nRF52 with user-defined up to 16b resolution * enable 6x PWM signals for nRF51 (up from 3) * enable 12x PWM signals for nRF52 (up from 3) * disable / free-up PWM signals when ZERO is written as the analog value analogWrite(PIN, 0)

micooke · 2017-10-09T03:54:21Z

Update :

6x analog/PWM signals enabled for nRF51 (up from 3)
12x analog/PWM signals enabled for nRF52 (up from 3)

The nRF51 now uses TIMER2 as well as TIMER1. As i have said before, the softdevice uses TIMER0 so i didn't touch that.

The nRF5x documentation is lacking pretty severely compared to the AVR chips I have dealt with. Im very surprised but the implementation for the nRF51 uses software to set/clear the hardware pins. It also uses the first channel as the period interrupt, rather than having a separate overflow interrupt (which can be seen in the RBL core and several Nordic source code).

It basically looks like PWM is implemented rather poorly in the nRF51, and i guess that is the reason that the nRF52 has dedicated PWM channels.

dlabun · 2018-02-27T22:58:33Z

@micooke Any issues with merging this fix?

sandeepmistry · 2018-02-27T23:01:11Z

Let’s hold off on this one for a bit ...

micooke · 2018-05-09T22:56:18Z

Hi @sandeepmistry, is there an issue with this PR I haven't addressed or does it just need more testing?

sandeepmistry · 2018-08-19T13:01:28Z

@micooke @dlabun this one needs some more testing.

I'm concerned of the performance implications of calling analogWrite(pin, 0) for every digitalWrite(pin, value) call.

micooke · 2018-08-19T22:11:18Z

The idea for that was actually copied from the Arduino AVR core
https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/wiring_analog.c#L112-L119

Which uses digitalWrite() if 0 or 255 are passed to analogWrite()

micooke changed the title ~~Fix bug #169~~ Fix bug #169 - analogWrite Sep 29, 2017

sandeepmistry reviewed Oct 2, 2017

View reviewed changes

micooke commented Oct 6, 2017

View reviewed changes

cores/nRF5/wiring_analog_nRF51.c Outdated

}

else

{

digitalWrite(ulPin, LOW);

Copy link

Contributor Author

micooke Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be HIGH - copy/paste error

micooke commented Oct 6, 2017

View reviewed changes

cores/nRF5/wiring_analog_nRF52.c Outdated

}

else

{

digitalWrite(ulPin, LOW);

Copy link

Contributor Author

micooke Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same copy/paste error again

sandeepmistry mentioned this pull request Sep 6, 2018

PWM signal from analogWrite does not stop when pin is set to different mode #169

Open

deadprogram mentioned this pull request Oct 4, 2018

NRF52: implement ADC and PWM interfaces tinygo-org/tinygo#32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug #169 - analogWrite #195

Fix bug #169 - analogWrite #195

micooke commented Sep 29, 2017

micooke commented Sep 29, 2017

sandeepmistry left a comment

sandeepmistry Oct 2, 2017

micooke Oct 6, 2017 •

edited

Loading

micooke Oct 6, 2017

sandeepmistry Oct 2, 2017

micooke Oct 6, 2017

micooke commented Oct 6, 2017

micooke commented Oct 6, 2017 •

edited

Loading

dlabun commented Oct 6, 2017

micooke Oct 6, 2017

micooke Oct 6, 2017

micooke commented Oct 9, 2017 •

edited

Loading

dlabun commented Feb 27, 2018

sandeepmistry commented Feb 27, 2018

micooke commented May 9, 2018

sandeepmistry commented Aug 19, 2018

micooke commented Aug 19, 2018 •

edited

Loading

		@@ -86,18 +93,42 @@ void digitalWrite( uint32_t ulPin, uint32_t ulVal )

		ulPin = g_ADigitalPinMap[ulPin];

		for (uint8_t i = 0; i < PWM_COUNT; i++)

Fix bug #169 - analogWrite #195

Are you sure you want to change the base?

Fix bug #169 - analogWrite #195

Conversation

micooke commented Sep 29, 2017

micooke commented Sep 29, 2017

sandeepmistry left a comment

Choose a reason for hiding this comment

sandeepmistry Oct 2, 2017

Choose a reason for hiding this comment

micooke Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

micooke Oct 6, 2017

Choose a reason for hiding this comment

sandeepmistry Oct 2, 2017

Choose a reason for hiding this comment

micooke Oct 6, 2017

Choose a reason for hiding this comment

micooke commented Oct 6, 2017

micooke commented Oct 6, 2017 • edited Loading

dlabun commented Oct 6, 2017

micooke Oct 6, 2017

Choose a reason for hiding this comment

micooke Oct 6, 2017

Choose a reason for hiding this comment

micooke commented Oct 9, 2017 • edited Loading

dlabun commented Feb 27, 2018

sandeepmistry commented Feb 27, 2018

micooke commented May 9, 2018

sandeepmistry commented Aug 19, 2018

micooke commented Aug 19, 2018 • edited Loading

micooke Oct 6, 2017 •

edited

Loading

micooke commented Oct 6, 2017 •

edited

Loading

micooke commented Oct 9, 2017 •

edited

Loading

micooke commented Aug 19, 2018 •

edited

Loading