cpu, atmega_common: add periph/adc by smlng · Pull Request #7954 · RIOT-OS/RIOT

smlng · 2017-11-07T20:33:45Z

based on #7227 as discussed with @PeterKietzmann, with missing comments addressed.

smlng · 2017-11-08T10:56:02Z

tested works for Arduino-mega2560

PeterKietzmann · 2017-11-08T10:58:50Z

Without looking at the code, my findings so far:

arduino-mega2560:

works as expected

arduino-duemilanove:

principally works as expected but it seems there are only 6 analog pins on that board though 8 seem to be initialized

waspmote:

xtimer does not fire (unrelated to ADC)
ADC_LINE(0) does not work or I connected the wrong pin (my pinout reference)
rest works as expected

PeterKietzmann · 2017-11-08T11:02:51Z

Waspmotes reference says: Waspmote has 7 accessible analog inputs in the sensor connector
-> might be the same problem as on the duemilanove

smlng · 2017-11-08T11:10:02Z

for the Arduino-uno we could explicitly set ADC_NUMOF (6U) to fix that, but for the waspmote its a bit complicated as line 0 isn't connected so setting ADC_NUMOF (7U) here wouldn't fix that. On the other hand, the corresponding CPUs do have 8 ADC lines but only 6 or 7 are accessible.

smlng · 2017-11-08T11:16:26Z

boards/waspmote-pro/include/periph_conf.h

+#define ADC_PIN_4           4
+#define ADC_PIN_5           5
+#define ADC_PIN_6           6
+#define ADC_PIN_7           7


@PeterKietzmann I think these names are ~~from~~ [wrong,] according to the waspmote manual they should be

ANALOG1 => 14 ANALOG2 => 15 ANALOG3 => 16 ANALOG4 => 17 ANALOG5 => 18 ANALOG6 => 19 ANALOG7 => 20

right? Though without the values, its just copy-paste from page 46.

@PeterKietzmann looking at page 45 of the waspmote manual there is a ANALOG pin top-left on the larger connector, maybe that's the missing ANALOG0?

Actually I don't even know why these 'pins' are defined here. ADC_DEV(x) returns x and should be used AFAIK. The ANALOG pin on the top left of the waspmote does not work. I'm against introducing the waspmote semantic to RIOTs periph_conf.h

but then something like val = analogRead(ANALOG1); as in the manual is not working.

when using an Arduino sketch with RIOT

Now that you addressed several arduino mappings in this PR, what is this needed for?

PeterKietzmann · 2017-11-08T12:57:25Z

I do not like the fact that we usually use `ADC_NUMOF'' et al as board specific (not MCU specific) indicator for a number of defined devices. As I see it here, it is used to indicate MCU specifics which must not be fully accomplished by a board. The reason for that is most likely the fact of missing configuration capabilities on such controllers. Am I right? However, I don't see this PR responsible to address that issue. Maybe a candidate for #7528?

smlng · 2017-11-08T12:58:30Z

still need to address murdock issues for nucleos

smlng · 2017-11-08T12:59:17Z

at least when they have ADC

PeterKietzmann · 2017-11-08T13:09:41Z

What you think about removing ADC_PIN_X definitions?

smlng · 2017-11-08T13:18:03Z

What you think about removing ADC_PIN_X definitions?

yes they can go, but I still think we should have ANALOG1-7 defined for waspmote instead.

PeterKietzmann · 2017-11-08T14:32:52Z

Why?

smlng · 2017-11-08T14:44:12Z

because otherwise any sketch for the waspmote will fail when compiled with RIOT. IIRC that's the idea of the Arduino feature in RIOT, that you can take an existing sketch and use it with RIOT, however, if one uses analogRead(ANALOG1); it would currently fail, or not?

PeterKietzmann · 2017-11-08T15:05:05Z

Yes it would fail. I'm not too familiar with the Arduino wrapper but (i) I would have assumed the mapping you propose as part of the arduino_pinmap.h file (not periph_conf.h) and (ii) looking at the referenced file it appears that Arduino pins are mapped like ARDUINO_PIN_X. Can you shed some light into the dark?

haukepetersen

Just one minor thing from my side...

haukepetersen · 2017-11-09T11:25:08Z

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

+#endif
+
+#ifdef CPU_ATMEGA1281
+#define ADC_NUMOF       (8)


maybe better

#if defined(CPU_ATMEGA328P) || defined(CPU_ATMEGA1281)

above?

smlng · 2017-11-09T23:00:43Z

@haukepetersen addressed your comment, and fixed issues reported by Murdock.

smlng · 2017-11-09T23:01:52Z

if someone could review the ADC mapping for those boards - I'm a bit unsure if they're all correct.

PeterKietzmann · 2017-11-10T07:50:01Z

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

+#define ARDUINO_A14             ADC_LINE(14)
+#define ARDUINO_A15             ADC_LINE(15)
+#endif
+


I still don't get this naming scheme, can you send a reference? Above, for instance, analog pins are named like ARDUINO_PIN_AX. In the Arduino forum it also appears that pins are addresses as AX.

IIRC its partly invented by @haukepetersen, see original PR by @dnahm #7227. This mapping is basically needed to support ADC in sys/arduino/base.cpp.

What I didn't know/see was that ARDUINO_PIN_AX seems to be used for digital I/Os and ARDUINO_AX for analog inputs. Fine with me...

right (I guess), otherwise there would be a name/macro clash between readAnalog and readDigitial and the corresponding pin mappings used in sys/arduino/base.cpp.

PeterKietzmann · 2017-11-10T07:55:12Z

boards/stm32f4discovery/include/arduino_pinmap.h

+ */
+
+/**
+ * @ingroup     boards_nucleo32-common


Discovery boards are not in nucleo common, are they?

not that I know of, copy-pasta 🍝

PeterKietzmann · 2017-11-10T08:00:06Z

boards/waspmote-pro/include/periph_conf.h

+#define ADC_PIN_4           4
+#define ADC_PIN_5           5
+#define ADC_PIN_6           6
+#define ADC_PIN_7           7


Now that you addressed several arduino mappings in this PR, what is this needed for?

smlng · 2017-11-10T08:31:44Z

yep, I think the ADC_PIN_X mappings are obsolete now.

smlng · 2017-11-10T08:38:51Z

Now that you addressed several arduino mappings in this PR, what is this needed for

mhm, I'm unsure, and would rather clean this up later on, I think this is related to #7528. So leave as is for now, it doesn't hurt and cleanup then.

PeterKietzmann · 2017-11-10T10:02:17Z

mhm, I'm unsure, and would rather clean this up later on, I think this is related to #7528. So leave as is for now, it doesn't hurt and cleanup then.

@smlng sure it doesn't hurt but on the other hand I think it makes the thing even worse as it introduces an inconsistency to the other boards

smlng · 2017-11-10T13:04:00Z

@PeterKietzmann IMHO analog pin mapping looks good for nucleos and the rest, I remove the extra (but unused) analog mappings as discussed offline.

May I squash?

PeterKietzmann · 2017-11-10T13:10:41Z

Please squash. I will give it a quick test run again then

PeterKietzmann

ACK

smlng · 2017-11-10T14:16:32Z

@haukepetersen all good, to get this finally merged?

PeterKietzmann · 2017-11-13T10:44:54Z

Thx! ACK and go

smlng mentioned this pull request Nov 7, 2017

cpu/atmega_common: add adc driver #7227

Closed

miri64 assigned haukepetersen and lebrush Nov 7, 2017

smlng force-pushed the cpu/atmega/adc branch from 221ae78 to f1bbae3 Compare November 8, 2017 10:57

smlng commented Nov 8, 2017

View reviewed changes

haukepetersen requested changes Nov 9, 2017

View reviewed changes

smlng force-pushed the cpu/atmega/adc branch from f1bbae3 to 1b0e1c7 Compare November 9, 2017 22:54

PeterKietzmann reviewed Nov 10, 2017

View reviewed changes

smlng force-pushed the cpu/atmega/adc branch from 36c9a7e to e6fbbf2 Compare November 10, 2017 13:01

cpu/atmega_common: add adc driver

9de00af

smlng force-pushed the cpu/atmega/adc branch from e6fbbf2 to 9de00af Compare November 10, 2017 13:19

PeterKietzmann approved these changes Nov 10, 2017

View reviewed changes

haukepetersen approved these changes Nov 13, 2017

View reviewed changes

PeterKietzmann merged commit 8999460 into RIOT-OS:master Nov 13, 2017

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

smlng deleted the cpu/atmega/adc branch February 6, 2018 19:43

Conversation

smlng commented Nov 7, 2017

Uh oh!

smlng commented Nov 8, 2017

Uh oh!

PeterKietzmann commented Nov 8, 2017

Uh oh!

PeterKietzmann commented Nov 8, 2017

Uh oh!

smlng commented Nov 8, 2017

Uh oh!

smlng Nov 8, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Nov 8, 2017

Uh oh!

smlng commented Nov 8, 2017

Uh oh!

smlng commented Nov 8, 2017

Uh oh!

PeterKietzmann commented Nov 8, 2017

Uh oh!

smlng commented Nov 8, 2017

Uh oh!

PeterKietzmann commented Nov 8, 2017

Uh oh!

smlng commented Nov 8, 2017

Uh oh!

PeterKietzmann commented Nov 8, 2017

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 9, 2017

Uh oh!

smlng commented Nov 9, 2017

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!

smlng commented Nov 10, 2017

Uh oh!

smlng commented Nov 10, 2017

Uh oh!

PeterKietzmann commented Nov 10, 2017

Uh oh!

smlng commented Nov 10, 2017

Uh oh!

PeterKietzmann commented Nov 10, 2017

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 10, 2017

Uh oh!

PeterKietzmann commented Nov 13, 2017

smlng Nov 8, 2017 •

edited

Loading