Skip to content

cpu/atmega_common: add adc driver #7227

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.

Sign up for GitHub

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

Closed
ghost wants to merge 1 commit into from
Closed

cpu/atmega_common: add adc driver #7227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions boards/arduino-atmega-common/Makefile.features
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Put defined MCU peripherals here (in alphabetical order)
FEATURES_PROVIDED += periph_adc
FEATURES_PROVIDED += periph_gpio
FEATURES_PROVIDED += periph_i2c
FEATURES_PROVIDED += periph_spi
Expand Down
24 changes: 24 additions & 0 deletions boards/arduino-atmega-common/include/arduino_board.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,30 @@ static const gpio_t arduino_pinmap[] = {
#endif
};

/**
* @brief Look-up table for the Arduino's analog pins
*/
static const adc_t arduino_analog_map[] = {
ARDUINO_A0,
ARDUINO_A1,
ARDUINO_A2,
ARDUINO_A3,
ARDUINO_A4,
ARDUINO_A5,
ARDUINO_A6,
ARDUINO_A7,
#ifdef CPU_ATMEGA2560
ARDUINO_A8,
ARDUINO_A9,
ARDUINO_A10,
ARDUINO_A11,
ARDUINO_A12,
ARDUINO_A13,
ARDUINO_A14,
ARDUINO_A15,
#endif
};

#ifdef __cplusplus
}
#endif
Expand Down
20 changes: 19 additions & 1 deletion boards/arduino-atmega-common/include/arduino_pinmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,27 @@ extern "C" {
#define ARDUINO_PIN_A14 ARDUINO_PIN_68
#define ARDUINO_PIN_A15 ARDUINO_PIN_69
#endif

Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Jul 13, 2017

Choose a reason for hiding this comment

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

you shoud re-add the ARDUINO_Ax defines!

/** @ */

#define ARDUINO_A0 ADC_LINE(0)
#define ARDUINO_A1 ADC_LINE(1)
#define ARDUINO_A2 ADC_LINE(2)
#define ARDUINO_A3 ADC_LINE(3)
#define ARDUINO_A4 ADC_LINE(4)
#define ARDUINO_A5 ADC_LINE(5)
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Aug 24, 2017

Choose a reason for hiding this comment

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

Code duplication here. You can omit the #ifdef CPU_ATMEGA328P above and define A0-A5 for all boards and then you optionally add A6-A15 for the mega2560...

Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

Atmega328p (arduino nano) has also lines A6 and A7, please move the following #ifdef two lines below

#define ARDUINO_A6 ADC_LINE(6)
#define ARDUINO_A7 ADC_LINE(7)
#ifdef CPU_ATMEGA2560
#define ARDUINO_A8 ADC_LINE(8)
#define ARDUINO_A9 ADC_LINE(9)
#define ARDUINO_A10 ADC_LINE(10)
#define ARDUINO_A11 ADC_LINE(11)
#define ARDUINO_A12 ADC_LINE(12)
#define ARDUINO_A13 ADC_LINE(13)
#define ARDUINO_A14 ADC_LINE(14)
#define ARDUINO_A15 ADC_LINE(15)
#endif

#ifdef __cplusplus
}
#endif
Expand Down
28 changes: 27 additions & 1 deletion boards/arduino-atmega-common/include/periph_conf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
* Copyright (C) 2014 Freie Universität Berlin, Hinnerk van Bruinehsen
* 2016 Laurent Navet <[email protected]>
* 2017 HAW Hamburg, Dimitri Nahm
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
Expand All @@ -17,6 +18,7 @@
* @author Hinnerk van Bruinehsen <[email protected]>
* @author Laurent Navet <[email protected]>
* @author Hauke Petersen <[email protected]>
* @author Dimitri Nahm <[email protected]>
*/

#ifndef PERIPH_CONF_H
Expand Down Expand Up @@ -136,9 +138,33 @@ extern "C" {
/** @} */

/**
* @brief I2C configuration
* @name I2C configuration
* @{
*/
#define I2C_NUMOF 1
/** @} */

/**
* @name ADC configuration
*
* The number of ADC channels of the atmega328p depends on the package:
* - 6-channel 10-bit ADC in PDIP package
* - 8-channel 10-bit ADC in TQFP and QFN/MLF package
* Arduino UNO / Duemilanove has thereby 6 channels. But only 5 channels can be
* used for ADC, because the pin of ADC5 emulate a software triggered interrupt.
* -> boards -> arduino-atmega-common -> include -> board_common.h
* @{
*/
#if defined (CPU_ATMEGA328P)
#define ADC_NUMOF (8)
#elif defined (CPU_ATMEGA2560)
#define ADC_NUMOF (16)
#endif

#ifdef CPU_ATMEGA1281
#define ADC_NUMOF (8)
#endif
/** @} */

#ifdef __cplusplus
}
Expand Down
1 change: 1 addition & 0 deletions boards/waspmote-pro/Makefile.features
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Put defined MCU peripherals here (in alphabetical order)
FEATURES_PROVIDED += periph_adc
FEATURES_PROVIDED += periph_gpio
FEATURES_PROVIDED += periph_i2c
FEATURES_PROVIDED += periph_spi
Expand Down
20 changes: 19 additions & 1 deletion boards/waspmote-pro/include/periph_conf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,27 @@ extern "C" {
/** @} */

/**
* @brief I2C configuration
* @name I2C configuration
* @{
*/
#define I2C_NUMOF 1
/** @} */

/**
* @name ADC configuration
* @{
*/
#define ADC_NUMOF 8
/* ADC Channels */
#define ADC_PIN_0 0
#define ADC_PIN_1 1
#define ADC_PIN_2 2
#define ADC_PIN_3 3
#define ADC_PIN_4 4
#define ADC_PIN_5 5
#define ADC_PIN_6 6
#define ADC_PIN_7 7
/** @} */
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Jun 29, 2017

Choose a reason for hiding this comment

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

same here, use ARDUINO_Ax ADC_LINE(x)

Copy link
Copy Markdown
Author

@ghost ghost Jun 30, 2017

Choose a reason for hiding this comment

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

It is no Arduino board. So I will remove ADC channel configuration.


#ifdef __cplusplus
}
Expand Down
145 changes: 145 additions & 0 deletions cpu/atmega_common/periph/adc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/*
* Copyright (C) 2016 Laurent Navet <[email protected]>
* 2017 HAW Hamburg, Dimitri Nahm
*
* This file is subject to the terms and conditions of the GNU Lesser General
* Public License v2.1. See the file LICENSE in the top level directory for more
* details.
*/

/**
* @ingroup drivers_periph
* @{
*
* @file
* @brief Low-level ADC driver implementation for ATmega family
*
* @author Laurent Navet <[email protected]>
* @author Dimitri Nahm <[email protected]>
*
* @}
*/

#include "cpu.h"
#include "mutex.h"
#include "assert.h"
#include "periph/adc.h"
#include "periph_conf.h"

#define ADC_MAX_CLK (200000U)

static mutex_t lock = MUTEX_INIT;

static inline void prep(void)
{
mutex_lock(&lock);
/* Enable ADC */
ADCSRA |= (1 << ADEN);
}

static inline void done(void)
{
/* Disable ADC */
ADCSRA &= ~(1 << ADEN);
mutex_unlock(&lock);
}

int adc_init(adc_t line)
{
uint32_t clk_div = 1;

/* check if the line is valid */
if (line >= ADC_NUMOF) {
return -1;
}

prep();
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

ADC doesn't need to be enabled here

Copy link
Copy Markdown
Author

@ghost ghost Aug 31, 2017

Choose a reason for hiding this comment

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

But it must be locked


/* Disable corresponding Digital input */
#if defined (CPU_ATMEGA328P) || defined (CPU_ATMEGA1281)
Copy link
Copy Markdown
Member

@smlng smlng Aug 24, 2017

Choose a reason for hiding this comment

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

indention if of by one tab for all #if statements following

DIDR0 |= (1 << line);
#elif defined (CPU_ATMEGA2560)
if (line < 8)
DIDR0 |= (1 << line);
else
DIDR2 |= (1 << (line - 8));
#endif

/* Set ADC-pin as input */
#if defined (CPU_ATMEGA328P)
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

why not simply calling gpio_init?

    if (line < 8) {
        gpio_init(GPIO_PIN(PORT_C, line), GPIO_IN);
    }
#ifndef CPU_ATMEGA328P
    else {
        gpio_init(GPIO_PIN(PORT_K, line - 8), GPIO_IN);
    }
#endif

DDRC &= ~(1 << line);
PORTC &= ~(1 << line);
#elif defined (CPU_ATMEGA2560)
if (line < 8) {
DDRF &= ~(1 << line);
PORTF &= ~(1 << line);
}
else {
DDRK &= ~(1 << (line-8));
PORTK &= ~(1 << (line-8));
}
#elif defined (CPU_ATMEGA1281)
DDRF &= ~(1 << line);
PORTF &= ~(1 << line);
#endif

/* set clock prescaler to get the maximal possible ADC clock value */
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

Idea: since prescalers just depends in the coreclock, can be set by the pre-processor

/* sampling freq must be between 200kHz and 50kHz */
#if CLOCK_CORECLOCK >  12800000
#define PRESCALER 0x07  /* 128 */
#elif CLOCK_CORECLOCK > 6400000
#define PRESCALER 0x06  /* 64 */
#elif CLOCK_CORECLOCK > 3200000
#define PRESCALER 0x05  /* 32 */
#elif CLOCK_CORECLOCK > 1600000
#define PRESCALER 0x04  /* 16 */
#elif CLOCK_CORECLOCK >  800000
#define PRESCALER 0x03  /* 8 */
#elif CLOCK_CORECLOCK >  100000
#define PRESCALER 0x01  /* 2 */
#else
#error CLOCK must be at least 100kHz to use ADC
#endif

Copy link
Copy Markdown
Member

@smlng smlng Sep 1, 2017

Choose a reason for hiding this comment

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

in #7502 its the other way around, in general I'd say the compiler should optimise that out anyway. But to be sure, you could make a size comparison.

However, I would leave as is for now.

for (clk_div = 1; clk_div < 8; clk_div++) {
if ((CLOCK_CORECLOCK / (1 << clk_div)) <= ADC_MAX_CLK) {
break;
}
}
ADCSRA |= clk_div;

/* Ref Voltage is Vcc(5V) */
ADMUX |= (1 << REFS0);
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

I wouldn't force it to 5V, it' might be interesting to have VREF as reference voltage. Plus in the 1281/2560 it can be set to differential measurements which might be quite useful.

I suggest introduce a configuration struct, which allows to configure it for each channel. Then set the reference when doing the triggering the sample.

typedef struct {
    gpio_t pin;                 
    adc_ref_t ref;               
    uint8_t ch;     
} adc_conf_t;

...

static const adc_conf_t adc_conf[] = {
    { GPIO_PIN(ADC_PORT, 0), ADC_REF_AVCC, 0 },
    { GPIO_PIN(ADC_PORT, 1), ADC_REF_AVCC, 1 },
    { GPIO_PIN(ADC_PORT, 2), ADC_REF_AVCC, 2 },
    ...
}

This also simplifies the adc_init and adc_sample code.

Copy link
Copy Markdown
Member

@smlng smlng Sep 28, 2017

Choose a reason for hiding this comment

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

In general a good idea, specifically to have the reference voltage configurable. What I don't like is the duplication in pin and ch because they have a direct dependency and ADC_PORT is fixed for all anyway.

Copy link
Copy Markdown
Member

@smlng smlng Sep 28, 2017

Choose a reason for hiding this comment

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

but maybe we should get general ADC support first and fix this later on. A first step could also be to make the ref voltage configurable via define macro (though it applies to all pins then).


done();

return 0;
}

int adc_sample(adc_t line, adc_res_t res)
{
int sample = 0;

/* check if resolution is applicable */
assert(res == ADC_RES_10BIT);
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

This should be a return. Documentation states:
@return -1 if resolution is not applicable


prep();
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

it has to be enabled after setting the other registers otherwise some of the registers of the 328p can't be written


/* set conversion channel */
#if defined (CPU_ATMEGA328P) || defined (CPU_ATMEGA1281)
ADMUX &= 0xf0;
ADMUX |= line;
#endif
#ifdef CPU_ATMEGA2560
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Aug 24, 2017

Choose a reason for hiding this comment

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

why not use a straight forward #elif defined(CPU_ATMEGA2560) here? This would be IMHO a little easier to read...

if(line < 8) {
Copy link
Copy Markdown
Member

@smlng smlng Aug 24, 2017

Choose a reason for hiding this comment

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

coding style, missing space after if

ADCSRB &= ~(1 << MUX5);
ADMUX &= 0xf0;
ADMUX |= line;
}
else {
ADCSRB |= (1 << MUX5);
ADMUX &= 0xf0;
ADMUX |= (line-8);
}
#endif

/* Start a new conversion. By default, this conversion will
be performed in single conversion mode. */
ADCSRA |= (1 << ADSC);

/* Wait until the conversion is complete */
while(ADCSRA & (1 << ADSC)) {}

/* Get conversion result */
sample = ADC;
Copy link
Copy Markdown
Member

@lebrush lebrush Aug 30, 2017

Choose a reason for hiding this comment

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

Result is 10 bits, you need to read two registers (in the correct order to get the result)

Copy link
Copy Markdown
Author

@ghost ghost Aug 31, 2017

Choose a reason for hiding this comment

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

Both registers (ADCL, ADCH) are read out via ADC register


/* Clear the ADIF flag */
ADCSRA |= (1 << ADIF);

done();

return sample;
}
34 changes: 34 additions & 0 deletions sys/arduino/base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,15 @@
extern "C" {
#include "xtimer.h"
#include "periph/gpio.h"
#ifdef ADC_NUMOF
#include "periph/adc.h"
#endif
}

#include "arduino.hpp"

#define ANALOG_PIN_NUMOF (sizeof(arduino_analog_map) / sizeof(arduino_analog_map[0]))

void pinMode(int pin, int mode)
{
gpio_mode_t m = GPIO_OUT;
Expand Down Expand Up @@ -58,3 +63,32 @@ void delay(unsigned long msec)
{
xtimer_usleep(1000 * msec);
}

/*
* Bitfield for the state of the ADC-channels.
* 0: Not initialized
* 1: Successfully initialized
*/
static uint16_t adc_line_state = 0;

int analogRead(int arduino_pin)
{
int adc_value;

/* Check if the ADC line is valid */
assert((arduino_pin >= 0) && (arduino_pin < ANALOG_PIN_NUMOF));

/* Initialization of given ADC channel */
if (!(adc_line_state & (1 << arduino_pin))) {
if (adc_init(arduino_analog_map[arduino_pin]) != 0) {
return -1;
}
/* The ADC channel is initialized */
adc_line_state |= 1 << arduino_pin;
}

/* Read the ADC channel */
adc_value = adc_sample(arduino_analog_map[arduino_pin], ADC_RES_10BIT);

return adc_value;
}
10 changes: 10 additions & 0 deletions sys/arduino/include/arduino.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,15 @@ int digitalRead(int pin);
*/
void delay(unsigned long msec);

/**
* @brief Read the current value of the given analog pin
*
* @param[in] pin pin to read
*
* @return a value between 0 to 1023 that is proportionnal
* to the voltage applied to the pin
Copy link
Copy Markdown
Member

@smlng smlng Aug 24, 2017

Choose a reason for hiding this comment

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

according the above, it may also return -1 or -2 on error

*/
int analogRead(int pin);

#endif /* ARDUINO_H */
/** @} */