Skip to content

Due: solve issues with nonstandard pin operations #3524

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

Merged
merged 5 commits into from
Jan 7, 2016
Merged

Due: solve issues with nonstandard pin operations #3524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9491c1f
Defaults output pins to LOW on Due
mtayler Jun 11, 2015
8589c79
Due: modify ADC enable/disable sequence
facchinm Mar 23, 2015
33255ef
Fix adc_configure_trigger enabling and disabling FreeRun Fix changing…
madrang Jan 25, 2014
d1c65ca
keep track of pin status using g_pinStatus array and act accordingly
facchinm Jul 14, 2015
5a37e94
fix leftover on pinMode(OUTPUT)
facchinm Jul 16, 2015
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
12 changes: 12 additions & 0 deletions hardware/arduino/sam/cores/arduino/Arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,16 @@ typedef enum _ETCChannel
#define PIN_ATTR_PWM (1UL<<3)
#define PIN_ATTR_TIMER (1UL<<4)

#define PIN_STATUS_DIGITAL_INPUT_PULLUP (0x01)
#define PIN_STATUS_DIGITAL_INPUT (0x02)
#define PIN_STATUS_DIGITAL_OUTPUT (0x03)
#define PIN_STATUS_ANALOG (0x04)
#define PIN_STATUS_PWM (0x05)
#define PIN_STATUS_TIMER (0x06)
#define PIN_STATUS_SERIAL (0x07)
#define PIN_STATUS_DW_LOW (0x10)
#define PIN_STATUS_DW_HIGH (0x11)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't these definitions be put in a typedef'ed enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

Being a mashup between an enum and a mask (to save some bytes) I'd prefer to keep the hardcoded values, but it's a matter of choice 😄


/* Types used for the tables below */
typedef struct _PinDescription
{
Expand All @@ -170,6 +180,8 @@ typedef struct _PinDescription
ETCChannel ulTCChannel ;
} PinDescription ;

extern uint8_t g_pinStatus[];

/* Pins table to be instanciated into variant.cpp */
extern const PinDescription g_APinDescription[] ;

Expand Down
17 changes: 7 additions & 10 deletions hardware/arduino/sam/cores/arduino/wiring_analog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,12 @@ uint32_t analogRead(uint32_t ulPin)
case ADC11 :

// Enable the corresponding channel
if (ulChannel != latestSelectedChannel) {
if (adc_get_channel_status(ADC, ulChannel) != 1) {
adc_enable_channel( ADC, ulChannel );
if ( latestSelectedChannel != (uint32_t)-1 )
if ( latestSelectedChannel != (uint32_t)-1 && ulChannel != latestSelectedChannel)
adc_disable_channel( ADC, latestSelectedChannel );
latestSelectedChannel = ulChannel;
g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0xF0) | PIN_STATUS_ANALOG;
}

// Start the ADC
Expand Down Expand Up @@ -189,13 +190,9 @@ static void TC_SetCMR_ChannelB(Tc *tc, uint32_t chan, uint32_t v)
}

static uint8_t PWMEnabled = 0;
static uint8_t pinEnabled[PINS_COUNT];
static uint8_t TCChanEnabled[] = {0, 0, 0, 0, 0, 0, 0, 0, 0};

void analogOutputInit(void) {
uint8_t i;
for (i=0; i<PINS_COUNT; i++)
pinEnabled[i] = 0;
}

// Right now, PWM output only works on the pins with
Expand Down Expand Up @@ -263,7 +260,7 @@ void analogWrite(uint32_t ulPin, uint32_t ulValue) {
}

uint32_t chan = g_APinDescription[ulPin].ulPWMChannel;
if (!pinEnabled[ulPin]) {
if ((g_pinStatus[ulPin] & 0xF) != PIN_STATUS_PWM) {
// Setup PWM for this pin
PIO_Configure(g_APinDescription[ulPin].pPort,
g_APinDescription[ulPin].ulPinType,
Expand All @@ -273,7 +270,7 @@ void analogWrite(uint32_t ulPin, uint32_t ulValue) {
PWMC_SetPeriod(PWM_INTERFACE, chan, PWM_MAX_DUTY_CYCLE);
PWMC_SetDutyCycle(PWM_INTERFACE, chan, ulValue);
PWMC_EnableChannel(PWM_INTERFACE, chan);
pinEnabled[ulPin] = 1;
g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0xF0) | PIN_STATUS_PWM;
}

PWMC_SetDutyCycle(PWM_INTERFACE, chan, ulValue);
Expand Down Expand Up @@ -328,12 +325,12 @@ void analogWrite(uint32_t ulPin, uint32_t ulValue) {
TC_SetCMR_ChannelB(chTC, chNo, TC_CMR_BCPB_CLEAR | TC_CMR_BCPC_SET);
}
}
if (!pinEnabled[ulPin]) {
if ((g_pinStatus[ulPin] & 0xF) != PIN_STATUS_PWM) {
PIO_Configure(g_APinDescription[ulPin].pPort,
g_APinDescription[ulPin].ulPinType,
g_APinDescription[ulPin].ulPin,
g_APinDescription[ulPin].ulPinConfiguration);
pinEnabled[ulPin] = 1;
g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0xF0) | PIN_STATUS_PWM;
}
if (!TCChanEnabled[interfaceID]) {
TC_Start(chTC, chNo);
Expand Down
34 changes: 33 additions & 1 deletion hardware/arduino/sam/cores/arduino/wiring_digital.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,20 @@ extern void pinMode( uint32_t ulPin, uint32_t ulMode )
return ;
}

if ((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_ANALOG)
{
adc_disable_channel( ADC, g_APinDescription[ulPin].ulADCChannelNumber);
}

if ((g_pinStatus[ulPin] & 0xF) < PIN_STATUS_DIGITAL_OUTPUT && g_pinStatus[ulPin] != 0)
{
// return if already configured in the right way
if (((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_DIGITAL_INPUT && ulMode == INPUT) ||
((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_DIGITAL_INPUT_PULLUP && ulMode == INPUT_PULLUP) ||
((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_DIGITAL_OUTPUT && ulMode == OUTPUT))
return;
}

switch ( ulMode )
{
case INPUT:
Expand All @@ -39,6 +53,7 @@ extern void pinMode( uint32_t ulPin, uint32_t ulMode )
PIO_INPUT,
g_APinDescription[ulPin].ulPin,
0 ) ;
g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0xF0) | PIN_STATUS_DIGITAL_INPUT;
break ;

case INPUT_PULLUP:
Expand All @@ -49,15 +64,18 @@ extern void pinMode( uint32_t ulPin, uint32_t ulMode )
PIO_INPUT,
g_APinDescription[ulPin].ulPin,
PIO_PULLUP ) ;
g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0xF0) | PIN_STATUS_DIGITAL_INPUT_PULLUP;
break ;

case OUTPUT:
PIO_Configure(
g_APinDescription[ulPin].pPort,
PIO_OUTPUT_1,
(g_pinStatus[ulPin] & 0xF0) >> 4 ? PIO_OUTPUT_1 : PIO_OUTPUT_0,
g_APinDescription[ulPin].ulPin,
g_APinDescription[ulPin].ulPinConfiguration ) ;

g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0xF0) | PIN_STATUS_DIGITAL_OUTPUT;

/* if all pins are output, disable PIO Controller clocking, reduce power consumption */
if ( g_APinDescription[ulPin].pPort->PIO_OSR == 0xffffffff )
{
Expand All @@ -78,6 +96,12 @@ extern void digitalWrite( uint32_t ulPin, uint32_t ulVal )
return ;
}

if ((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_PWM) {
pinMode(ulPin, OUTPUT);
}

g_pinStatus[ulPin] = (g_pinStatus[ulPin] & 0x0F) | (ulVal << 4) ;

if ( PIO_GetOutputDataStatus( g_APinDescription[ulPin].pPort, g_APinDescription[ulPin].ulPin ) == 0 )
{
PIO_PullUp( g_APinDescription[ulPin].pPort, g_APinDescription[ulPin].ulPin, ulVal ) ;
Expand All @@ -90,6 +114,14 @@ extern void digitalWrite( uint32_t ulPin, uint32_t ulVal )

extern int digitalRead( uint32_t ulPin )
{
if ((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_DIGITAL_OUTPUT) {
return (g_pinStatus[ulPin] & 0xF0) >> 4;
}

if ((g_pinStatus[ulPin] & 0xF) == PIN_STATUS_ANALOG) {
pinMode(ulPin, INPUT);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the meaning of calling pinMode here?
I may have missed something but I understood the PIN_STATUS_ANALOG means the pin is under control of ADC or DAC. So, calling pinMode() which activate the clock on PIOx and put the PIOx pin as INPUT will run useless code.

Edit: I finally understood: the meaning is to commute the function of a pin from analog to digital. I feel a bit weird seeing the system pins configured during setup and their functions updated on the fly later during the main process but it may happen, indeed.

The secondary goal being to have this commutation done a fast as possible, the clock activation of the given PIOx should be done at once during the system inits and never touched again later. It is the way we followed for the Zero.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, the main goal of this patchset was solving the incompatibilities between pins behaviour on AVR and on SAM core, losing as few performances as we can.
For sure this piece of code can be optimized but, as you said, it's really uncommon to use pinMode during the loop, so we can be quite sure that it won't affect most people.

}

if ( g_APinDescription[ulPin].ulPinType == PIO_NOT_A_PIN )
{
return LOW ;
Expand Down
27 changes: 24 additions & 3 deletions hardware/arduino/sam/system/libsam/source/adc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,18 +192,39 @@ void adc_set_resolution(Adc *p_adc,const enum adc_resolution_t resolution)
void adc_configure_trigger(Adc *p_adc, const enum adc_trigger_t trigger,
uint8_t uc_freerun)
{
p_adc->ADC_MR |= trigger | ((uc_freerun << 7) & ADC_MR_FREERUN);
//Warning ADC_MR_TRGSEL_Msk does not include ADC_MR_TRGEN.
p_adc->ADC_MR &= ~(ADC_MR_TRGEN | ADC_MR_TRGSEL_Msk | ADC_MR_FREERUN); //Clear all bits related to triggers and freerun

//Configure FreeRun
if(uc_freerun & ADC_MR_FREERUN == ADC_MR_FREERUN_ON) { //FreeRun is enabled
p_adc->ADC_MR |= ADC_MR_FREERUN_ON;

//Free Run Mode: Never wait for any trigger
//No need to continue and enable hardware triggers
return;
}

//Configure hardware triggers
if(trigger & ADC_MR_TRGEN == ADC_MR_TRGEN_EN) { //Hardware trigger is enabled
p_adc->ADC_MR |= (trigger & ADC_MR_TRGSEL_Msk) | ADC_MR_TRGEN_EN; //Set trigger selection bits and enable hardware trigger
}
}
#elif SAM3U_SERIES
/**
* \brief Configure conversion trigger and free run mode.
* \brief Configure conversion trigger.
*
* \param p_adc Pointer to an ADC instance.
* \param trigger Conversion trigger.
*/
void adc_configure_trigger(Adc *p_adc, const enum adc_trigger_t trigger)
{
p_adc->ADC_MR |= trigger;
//Warning ADC_MR_TRGSEL_Msk does not include ADC_MR_TRGEN.
p_adc->ADC_MR &= ~(ADC_MR_TRGEN | ADC_MR_TRGSEL_Msk); //Clear all bits related to triggers

//Configure hardware triggers
if(trigger & ADC_MR_TRGEN == ADC_MR_TRGEN_EN) { //Hardware trigger is enabled
p_adc->ADC_MR |= (trigger & ADC_MR_TRGSEL_Msk) | ADC_MR_TRGEN_EN; //Set trigger selection bits and enable hardware trigger
}
}
#endif

Expand Down
3 changes: 3 additions & 0 deletions hardware/arduino/sam/variants/arduino_due_x/variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,9 @@ extern const PinDescription g_APinDescription[]=
{ NULL, 0, 0, PIO_NOT_A_PIN, PIO_DEFAULT, 0, NO_ADC, NO_ADC, NOT_ON_PWM, NOT_ON_TIMER }
} ;


uint8_t g_pinStatus[PINS_COUNT] = {0};

#ifdef __cplusplus
}
#endif
Expand Down