sys/arduino: add implementation for analogWrite by aabadie · Pull Request #12523 · RIOT-OS/RIOT

aabadie · 2019-10-21T08:29:24Z

Contribution description

This PR adds an implementation for the analogWrite function of the Arduino API. It's based on the PWM peripheral interface of RIOT.

The arduino_pwm feature is introduced to only have to provide this implementation on a subset of boards (arduino-zero, sodaq-automono and arduino-mkrs) which already provides arduino and periph_pwm features. Other boards can then be adapted later: Arduino atmega, STM32 nucleos, etc.

Since there's no one to one mapping possible between Arduino PWM pins and RIOT, there's a not so nice logic to look for an index in a list of pin/PWM dev/channel mapping. I tried to use designated initializers but doesn't build with c++ unless empty elements are also initialized with dummy values. And it makes the array static initialization quite ugly.

Testing procedure

Plug a LED on D9 and flash the following Arduino sketch on arduino-zero:

int value = 0;
int ledPwm = 9;

void setup(void)
{}

void loop(void)
{
    analogWrite(ledPwm, value);
    value = (value + 10) % 256;
    // wait a bit
    delay(50);
}

And watch it oscillate.

Issues/PRs references

Now based on #12541

haukepetersen

Nice effort! Just some things that should be clarified before this can be merged.

haukepetersen · 2019-10-21T09:52:59Z

boards/sodaq-autonomo/include/arduino_board.h

+    int pin;        /**< Arduino pin number */
+    int dev;        /**< PWM device index of pin */
+    int chan;       /**< PWM channel index */
+} arduino_pwm_t;


Any reason this type is (a) duplicated and (b) not defined somewhere more fitting, e.g. at least in the CPU if not in the global Arduino module?

I would say a bit of laziness and the fact that I was not sure where this would fit better. The mapping could depend on the underlying PWM implementation.

The periph_pwm API defines PWM devices and channels of these devices. IMHO, a PWM channel is completely specified by the tupel (dev, chan) which is routed to pin. The struct should fit all. platforms. If not, the platform could extend the structure.

exactly :-)

I moved the struct to periph/pwm.h, guarded by MODULE_ARDUINO. Not sure if it's the best place. It cannot be in arduino.hpp since this one already includes arduino_board.h...

haukepetersen · 2019-10-21T09:56:30Z

sys/arduino/base.cpp

+
+    /* Check if the given value is correct */
+    assert(value >= 0);
+    assert(value < 256);


This seems dangerous to me, triggering a assertion on a parameters out of range situation can IMHO be solved at runtime -> although the Arduino API does not say anything about this (see https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite), I would expect it is just setting the PWM to min/max value whenever an arg is given out of range?! This would be the more desirable behavior I'd say.

Agreed, I'll change that.

haukepetersen · 2019-10-21T09:57:55Z

sys/arduino/base.cpp

+#if MODULE_PERIPH_PWM
+#define PWM_MODE            PWM_LEFT
+#define PWM_FREQU           (1000U)
+#define PWM_STEPS           (256U)


Not sure to define these values statically here in the code is the best idea. It seems, that the actually Arduino API defines these dependent on the target board (see https://www.arduino.cc/reference/en/language/functions/analog-io/analogwrite/), so we might at least allow for redefinition for RIOT?!

You are right, I also read that frequency values depend on the boards in Arduino. This could be moved elsewhere: default defines in the Arduino.hpp and values overridden in boards to match the ones in official Arduino.

sounds great

Until now, there is no Arduino.hpp. It will be introduced for compatibility reasons with PR #10592 which is waiting for merge. But Arduino.hpp will be just a wrapper and all definitions should got to arduino.hpp.

I moved the defines in arduino.hpp (don't worry @gschorcht, there was a typo in my previous comment). Only frequency can be overridden. The other values seems to be fixed in Arduino.

haukepetersen · 2019-10-21T10:00:16Z

sys/arduino/base.cpp

+    * 0: Not initialized
+    * 1: Successfully initialized
+    */
+    static uint16_t pwm_dev_state;


I fear its not quite that simple: If one would call a digital_write() on that same pin in the mean time, the bit value in this variable is not valid anymore. So it seems to me, that this value must be shared for all functions in this module and it must be also updated by other functions using these pins...

Not sure, here it's only used to check if the PWM device is already initialized. It has nothing to do with the pin itself.

but pwm_init also takes care of configuring the output state/mode for the effected pin. So maybe this might be a more generic problem. What happens for code like the following in Arduino?

pinMode(X, OUTPUT); digitalWrite(X, HIGH); analogWtrite(X, 100); pinMode(X, OUTPUT); digitalWrite(X, HIGH); analogWrite(X, 50);

Here one would expect, that the wave form at pin X is active after the last analogWrite call. Now the bitfield above will actually prevent that from happen, as it thinks the pin is already initialized as PWM output, which is not the case anymore...

You are right. Looking at the samd21 implementation of analogWrite in Arduino, it seems the GPIO is reinitialized all time but not the TCC. This is a design incompatibility between Arduino and RIOT here.

Considering that Arduino is not really geared towards maximum effenciency, I think we should be good with re-initializing the PWM on every call, right? IMHO API complience is more important than efficiency considerations.

Only question is how to/if we need to deal with signal ripples at the pin that could potentially be caused by this re-initialization. But the exact same behavior is triggered on the very first call in any case, so we might just be able to ignore that issue.

If the function is necessary, shouldn't it be part of the PWM API. It's a bit weird that then name of a function in the GPIO API starts with pwm.

It's added in the pwm API but maybe the name is not good.

Yes, of course 😕 Sorry, my fault. For any reason I saw it in gpio.h.

It could be called pwm_channel_init instead ?

Not sure, while pwm_gpio_init says what it does, pwm_channel_init would be more consistent with terminology in pwm.h. Maybe, pwm_channel_init would be better.

pinMode(x OUTPUT); digitalWrite(x, HIGH); analogWrite(y, 128); digitalWrite(x, LOW);

This won't work if X and Y are 2 pins configured for 2 channels on the same PWM device because they are reinitialized for PWM in the first call to analogWrite. The second call to digitalWrite might not work (untested but with the current implementation that should be the case).

It could work with your proposed change to split the functionality of pwm_init Into pwm_dev_init and pwm_channel_init. What if PWM channels were never initialized in pwm_init, but always in pwm_set when a certain channel is used for the first time?

What if PWM channels were never initialized in pwm_init, but always in pwm_set when a certain channel is used for the first time?

Doing this will make pwm_set slightly less efficient but it will also be a little more robust (if the same pin is initialized differently between 2 calls to pwm_set.

May I suggest to drop the refactoring proposal from this PR and work on this separately ?

Doing this will make pwm_set slightly less efficient

Yes, but how often is it called in real applications? I woud guess not so often so that this overhead seems to be acceptable.

May I suggest to drop the refactoring proposal from this PR and work on this separately ?

Sure.

but how often is it called in real applications? I woud guess not so often so that this overhead seems to be acceptable.

I tend to agree here. And it's just 2 extra function calls. Doing this change, e.g not initialize all channels in pwm_init and only initialize one channel on demand in pwm_set, would also save 52 bytes on samd21...

gschorcht · 2019-10-21T11:58:58Z

@aabadie Would it make sense to provide a small test app together with this PR, lets say arduino_pwm?

aabadie · 2019-10-21T13:58:17Z

Would it make sense to provide a small test app together with this PR, lets say arduino_pwm?

Sounds good. Maybe an arduino_analog that would use both analogRead and analogWrite ?

aabadie · 2019-10-21T19:41:18Z

@gschorcht, I added a test sketch for the analogRead/analogWrite functions. It's untested (I don't have hardware with me for the moment).

@haukepetersen, regarding the idempotence of analogWrite, I pushed a couple of commits adding a new pwm_gpio_init function in the periph PWM API. The idea is to have a way to initialize the gpio, regardless of the underlying timer (like Arduino does). The PWM device is initialized once but the GPIO is reinitialized each time analogWrite is called. It's currently untested (for the same reason as above).

gschorcht · 2019-10-22T07:21:20Z

@gschorcht, I added a test sketch for the analogRead/analogWrite functions. It's untested (I don't have hardware with me for the moment).

Great. I have 8 hours of lectures today. I will test it later today, hopefully.

aabadie · 2019-10-22T10:38:59Z

I fixed a couple of issues and could make the test sketch work on arduino-zero.

gschorcht · 2019-10-23T01:01:03Z

When I started to test I realized that my Arduino Mega isn't supported and only Sam21D boards are supported. How difficult would it be also to support ATmega boards? What will be the process to support other platforms, e.g., ESP32?

aabadie · 2019-10-23T06:23:54Z

How difficult would it be also to support ATmega boards?

It should not be too difficult. I can adapt the PWM mapping for them.

aabadie · 2019-10-23T15:05:37Z

@gschorcht, I pushed an update adding the mapping for arduino atmega boards. Not tested (and won't be able to test in the next 10 days).

gschorcht · 2019-10-23T16:16:21Z

@gschorcht, I pushed an update adding the mapping for arduino atmega boards. Not tested (and won't be able to test in the next 10 days).

Thanks. I will test it ASAP.

boards/common/arduino-atmega/include/arduino_board.h

sys/arduino/base.cpp

tests/sys_arduino_analog/arduino-test.sketch

gschorcht · 2019-10-25T15:36:29Z

@aabadie After changing the led pin to 13, the test works on my Arduino Mega2560.

boards/common/arduino-atmega/include/arduino_board.h

gschorcht · 2019-10-26T08:54:48Z

@haukepetersen Are the changes ok for you?

The goal of this application is to test the analogRead and analogWrite Arduino function

aabadie · 2019-11-15T16:55:45Z

@haukepetersen Are the changes ok for you?

ping @haukepetersen :)

aabadie · 2019-11-19T14:41:23Z

ping @haukepetersen :)

@haukepetersen, do you still have concerns here ?

aabadie · 2019-11-21T13:35:21Z

@haukepetersen gentle ping :)

haukepetersen

Looks good enough to me, so no further concerns.

gschorcht · 2019-11-21T14:09:11Z

@aabadie Thanks for contributing the analogWrite. The degree of compatibility increase further.

aabadie · 2019-11-21T15:54:51Z

Many thanks guys!

aabadie added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: arduino API Area: Arduino wrapper API labels Oct 21, 2019

aabadie requested review from fjmolinas and gschorcht October 21, 2019 08:29

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 21, 2019

haukepetersen requested changes Oct 21, 2019

View reviewed changes

aabadie force-pushed the pr/sys/arduino_pwm branch from 005c514 to 59d692e Compare October 21, 2019 13:50

aabadie force-pushed the pr/sys/arduino_pwm branch 2 times, most recently from ce616bd to d60e037 Compare October 21, 2019 19:37

aabadie force-pushed the pr/sys/arduino_pwm branch from d60e037 to aea9827 Compare October 22, 2019 07:17

aabadie mentioned this pull request Oct 22, 2019

boards/arduino-zero: configure ADC channels #12541

Merged

aabadie force-pushed the pr/sys/arduino_pwm branch from aea9827 to eabfb35 Compare October 22, 2019 10:33

aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 22, 2019

aabadie removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 22, 2019

aabadie force-pushed the pr/sys/arduino_pwm branch from eabfb35 to e4972d7 Compare October 22, 2019 16:50

aabadie removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 22, 2019

aabadie force-pushed the pr/sys/arduino_pwm branch from e4972d7 to c3606ef Compare October 23, 2019 15:04

gschorcht requested changes Oct 25, 2019

View reviewed changes

boards/common/arduino-atmega/include/arduino_board.h Show resolved Hide resolved

sys/arduino/base.cpp Outdated Show resolved Hide resolved

gschorcht reviewed Oct 25, 2019

View reviewed changes

tests/sys_arduino_analog/arduino-test.sketch Outdated Show resolved Hide resolved

aabadie force-pushed the pr/sys/arduino_pwm branch from c3606ef to 12fca9e Compare October 25, 2019 15:44

aabadie force-pushed the pr/sys/arduino_pwm branch from 12fca9e to 8bb64e2 Compare October 25, 2019 16:05

gschorcht requested changes Oct 26, 2019

View reviewed changes

boards/common/arduino-atmega/include/arduino_board.h Outdated Show resolved Hide resolved

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 8, 2019

aabadie force-pushed the pr/sys/arduino_pwm branch from aa0e617 to 32da842 Compare November 8, 2019 18:57

aabadie added 7 commits November 15, 2019 17:51

drivers/periph_pwm: define mapping struct for arduino pins

abfa731

sys/arduino: provide implementation for analogWrite

0845cac

boards/arduino-zero: add mapping for Arduino PWM pins

f1cb147

boards/arduino-mkr: add mapping for Arduino PWM pins

d03c982

boards/sodaq-autonomo: add mapping for Arduino PWM pins

e0f2884

tests/sys_arduino_analog: add test application

779e25b

The goal of this application is to test the analogRead and analogWrite Arduino function

boards/arduino-atmega: add mapping for Arduino PWM pins

65e0247

aabadie force-pushed the pr/sys/arduino_pwm branch from 32da842 to 65e0247 Compare November 15, 2019 16:54

gschorcht approved these changes Nov 16, 2019

View reviewed changes

haukepetersen approved these changes Nov 21, 2019

View reviewed changes

gschorcht merged commit a05027c into RIOT-OS:master Nov 21, 2019

aabadie deleted the pr/sys/arduino_pwm branch November 21, 2019 15:54

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019

Conversation

aabadie commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschorcht Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Oct 21, 2019

Uh oh!

aabadie commented Oct 21, 2019

Uh oh!

aabadie commented Oct 21, 2019

Uh oh!

gschorcht commented Oct 22, 2019

Uh oh!

aabadie commented Oct 22, 2019

Uh oh!

gschorcht commented Oct 23, 2019

Uh oh!

aabadie commented Oct 23, 2019

Uh oh!

aabadie commented Oct 23, 2019

Uh oh!

gschorcht commented Oct 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gschorcht commented Oct 25, 2019

aabadie commented Oct 21, 2019 •

edited

Loading

aabadie Oct 21, 2019 •

edited

Loading

gschorcht Oct 25, 2019 •

edited

Loading