Due: solve issues with nonstandard pin operations #3524

facchinm · 2015-07-14T15:51:48Z

This PR solves the following Due broken behaviours:

Real issues:
1- pinMode(OUTPUT) -> analogWrite() -> digitalWrite() [ digital write is not executed ]
2- pinMode(OUTPUT) -> analogWrite() [if executed more than once] Inconsistent behaviors of analogWrite and pinMode on Due and Uno #2455
3- pinMode(OUTPUT) -> digitalWrite() -> pinMode(INPUT) -> digitalRead() [if executed more than once] Due: modify ADC enable/disable sequence #2823 Bug: analogRead() for Arduino Due should disable previous ADC channel before enabling new one #3064 Due bug: Cannot use pin as digital after analogRead #2198
4- pinMode(INPUT) -> analogRead() -> digitalRead()
AVR core compatibility issues:
5- pinMode(OUTPUT) [ defaults to HIGH ] Defaults output pins to LOW on Due #3317 pinMode OUTPUT sets HIGH after digitalWrite set LOW on Arduino Due #3310
6- pinMode(OUTPUT) -> digitalRead() [ always return 0 ] Due: digitalRead of output pin always returns 0, should return pin state for compatibility with '328 version #1597 Arduino DUE digitalRead when pinMode is OUTPUT #1533
7- digitalWrite() -> pinMode(OUTPUT) [ does not retain the intended value ]

The Due has lots of RAM so the solution uses 80 bytes to keep track of the pinmux state. A lot of AVR behaviours are "emulated" using this array.

The solution has been benchmarked and the speed tradeoff is:

-10% on pure digitalWrite and digitalRead
-5% to -25% on pure analogRead()
-400% on analogRead()followed by digitalWrite() on different pins 😱

All other cases were broken with the actual code so it's useless to test them

When a pin is designated as an output on the Arduino Due, the pin is set to a HIGH logic level. Changing the default pin state to LOW makes the behaviour correspond with AVR.

to avoid the bug arduino#2198 simply reconfigure the pin -> no additional overhead if pinMode configuration is performed at the beginning of the sketch, 4 to 25% overhead on all analogRead() due to the additional check

… trigger type

This commit adds 80 bytes to RAM usage and some overhead in normal operations.

facchinm · 2015-07-14T15:53:09Z

It's important to notice that every core based on Due should add the g_pinStatus array to its variant.cpp

facchinm · 2015-07-15T10:01:39Z

BTW, the 400% performance drop can be solved completely if pinMode(INPUT) and pinMode(OUTPUT) are performed outside loop() (which is a best practice in case of different static pins)

facchinm · 2015-07-15T13:10:47Z

@HanaJin @trickedj @Tamul @DavidEGrayson @JiapengLi @billroy @3esmit , could you please test the PR? Could it be a good solution for your issues?

3esmit · 2015-07-15T13:43:26Z

Test is not possible, Arduino SAM Boards module not found in the pull request file, there is no 'sam' folder inside 'hardware/arduino' in pull_requests/arduino-PR-3524-BUILD-361-windows.zip

Download Arduino SAM Boards (1.6.4) files from Board Manager and test it anyway?

3esmit · 2015-07-15T19:58:02Z

But looking at the code changed, one line is exactly what I did to fix it. But in #3310 @DavidEGrayson realised that is not a "default output state" problem. The problem is that if you send digitalWrite(LOW) before pinMode(OUTPUT) arduino will not output LOW, will output the initial output state, that by default is HIGH.

DavidEGrayson · 2015-07-15T20:12:38Z

I'm not sure what @3esmit is talking about in the last comment. This pull request adds the following code in pinMode(x, OUTPUT) (in wiring_digital.c) to determine if the output should be high or low:

g_pinStatus[ulPin] = ((g_pinStatus[ulPin] & 0xF0) >> 4 ? PIO_OUTPUT_1 : PIO_OUTPUT_0)

That's a step in the right direction. I haven't looked carefully at the rest of the code or tested any of it.

DavidEGrayson · 2015-07-15T20:14:06Z

hardware/arduino/sam/cores/arduino/wiring_digital.c

        break ;

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


There is no need to set g_pinStatus if you are going to do it again on the next line. There is no need to do & 0xF0 because those bits you are masking off would get shifted anyway.

I didn't checked all diffs, I'm not that deep into arduino code, but your comments makes sense. Thanks for the note.

@DavidEGrayson , I'm aware there's no need to mask the variable but all the patch is explicitly masked so there's no need to remove it only here (I bet it will be removed by the compiler anyway, but I'll check later). If you wish something fancier it could be converted to use bit fields, but there won't be any speed improvement

You also spotted a leftover that made its way into the PR , thank you 😄 . Fixed with 5a37e94

facchinm · 2015-07-16T07:55:46Z

Sorry for the inconvenience guys, the board manager is a great beast but, being the Due core still inside the IDE, it takes extra care to generate a custom core based on this PR (like we already do with the Zero). More news later today!

aethaniel · 2015-08-11T13:20:42Z

hardware/arduino/sam/cores/arduino/Arduino.h

+#define PIN_STATUS_TIMER                 (0x06)
+#define PIN_STATUS_SERIAL                (0x07)
+#define PIN_STATUS_DW_LOW                (0x10)
+#define PIN_STATUS_DW_HIGH               (0x11)


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

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 😄

DenDanny · 2015-08-28T15:30:30Z

@facchinm et al.

I was wondering if you could give this beginner a push in the right direction..

I want to switch an Analogue pin between INPUT and OUTPUT in order to switch a LED between a "receiving" and emitting state (LedComm) - which since 1.5.4 don't work no more. Also, i want to use multiple analogue pins (each with an LED).

I can sort of follow (in theory) what's been going on, but practically I have no idea where/how I can these fixes and patches you (and others) seem to have created. Where/what can I download to obtain these fixes?

Regards,
Danny
PS. see my example (that doesn't work) on: https://forum.arduino.cc/index.php?topic=344560.0

facchinm · 2015-08-31T16:06:22Z

Hi @DenDanny, the PR builder for Due core is still far from being complete, so in the meantime, if you wish to test this PR, I'd suggest to extract this file into sketchbook/hardware folder. A new menu entry (called Arduino ARM (32-bits) Boards PR3524) should appear the next time you restart the IDE, and it contains all the patches.

@HanaJin @trickedj @Tamul @DavidEGrayson @JiapengLi @billroy @3esmit sorry for the long delay, if you could test using this quick and dirty method it would be a great help for inclusion. Sorry again.

HanaJin · 2015-09-08T20:00:23Z

Hi @facchinm , I tempted to try the patches, but I got this error when I tried to upload the sketch. Please help.

facchinm · 2015-09-09T07:30:39Z

Hi @HanaJin , the packet contains only the core, not the tools, so you need to download the SAM core first using Board manager and then the IDE will know where to find the compiler.
Sorry for not explaining this in the previous post and thanks for testing this patch!

HanaJin · 2015-09-09T18:40:52Z

Hi @facchinm sorry to bug you again. I've downloaded the core and tested the original Due which compils fine, but with the patches, it still does not compile.

Note that I checked the .c.o file was right in the build folder.

facchinm · 2015-09-10T08:15:43Z

Ok, found, the source of your problems is the IDE and more specifically platform.keys.rewrite.txt file. The fastest (and safest) method to fix compilation is to use nightly build (https://www.arduino.cc/download.php?f=/arduino-nightly-windows.zip) 😉

facchinm · 2015-09-10T16:13:04Z

To test the PR completely, here you can find a board-to-board test with wiring instructions on top.

HanaJin · 2015-09-10T17:26:13Z

Yes, the patches fix the issue I described in #2455. Now I get similar results. Hope this helps.

Due: solve issues with nonstandard pin operations

mtayler and others added 4 commits July 14, 2015 10:34

Defaults output pins to LOW on Due

9491c1f

When a pin is designated as an output on the Arduino Due, the pin is set to a HIGH logic level. Changing the default pin state to LOW makes the behaviour correspond with AVR.

Due: modify ADC enable/disable sequence

8589c79

to avoid the bug arduino#2198 simply reconfigure the pin -> no additional overhead if pinMode configuration is performed at the beginning of the sketch, 4 to 25% overhead on all analogRead() due to the additional check

Fix adc_configure_trigger enabling and disabling FreeRun Fix changing…

33255ef

… trigger type

keep track of pin status using g_pinStatus array and act accordingly

d1c65ca

This commit adds 80 bytes to RAM usage and some overhead in normal operations.

facchinm added the Board: Arduino Due Applies only to the Due label Jul 14, 2015

facchinm assigned cmaglie Jul 14, 2015

cmaglie mentioned this pull request Jul 15, 2015

pinMode OUTPUT sets HIGH after digitalWrite set LOW on Arduino Due #3310

Closed

DavidEGrayson reviewed Jul 15, 2015
View reviewed changes

fix leftover on pinMode(OUTPUT)

5a37e94

facchinm mentioned this pull request Aug 10, 2015

Due: modify ADC enable/disable sequence #2823

Closed

aethaniel reviewed Aug 11, 2015
View reviewed changes

facchinm mentioned this pull request Sep 29, 2015

Arduino Due watchdog support 1.6.5 #3889

Closed

facchinm added a commit that referenced this pull request Jan 7, 2016

Merge pull request #3524 from facchinm/pins_DUE

74c70ba

Due: solve issues with nonstandard pin operations

facchinm merged commit 74c70ba into arduino:master Jan 7, 2016

sandeepmistry mentioned this pull request Mar 14, 2016

Arduino due SAM Boards - Pin 13 malfunction #4682

Closed

facchinm deleted the pins_DUE branch January 4, 2017 15:40

ollie1400 pushed a commit to ollie1400/Arduino that referenced this pull request May 2, 2022

Merge pull request arduino#3524 from facchinm/pins_DUE

86a34c7

Due: solve issues with nonstandard pin operations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Due: solve issues with nonstandard pin operations #3524

Due: solve issues with nonstandard pin operations #3524

facchinm commented Jul 14, 2015

facchinm commented Jul 14, 2015

facchinm commented Jul 15, 2015

facchinm commented Jul 15, 2015

3esmit commented Jul 15, 2015

3esmit commented Jul 15, 2015

DavidEGrayson commented Jul 15, 2015

DavidEGrayson Jul 15, 2015

3esmit Jul 15, 2015

facchinm Jul 16, 2015

facchinm Jul 16, 2015

facchinm commented Jul 16, 2015

aethaniel Aug 11, 2015

facchinm Aug 11, 2015

DenDanny commented Aug 28, 2015

facchinm commented Aug 31, 2015

HanaJin commented Sep 8, 2015

facchinm commented Sep 9, 2015

HanaJin commented Sep 9, 2015

facchinm commented Sep 10, 2015

facchinm commented Sep 10, 2015

HanaJin commented Sep 10, 2015

Due: solve issues with nonstandard pin operations #3524

Due: solve issues with nonstandard pin operations #3524

Conversation

facchinm commented Jul 14, 2015

facchinm commented Jul 14, 2015

facchinm commented Jul 15, 2015

facchinm commented Jul 15, 2015

3esmit commented Jul 15, 2015

3esmit commented Jul 15, 2015

DavidEGrayson commented Jul 15, 2015

DavidEGrayson Jul 15, 2015

Choose a reason for hiding this comment

3esmit Jul 15, 2015

Choose a reason for hiding this comment

facchinm Jul 16, 2015

Choose a reason for hiding this comment

facchinm Jul 16, 2015

Choose a reason for hiding this comment

facchinm commented Jul 16, 2015

aethaniel Aug 11, 2015

Choose a reason for hiding this comment

facchinm Aug 11, 2015

Choose a reason for hiding this comment

DenDanny commented Aug 28, 2015

facchinm commented Aug 31, 2015

HanaJin commented Sep 8, 2015

facchinm commented Sep 9, 2015

HanaJin commented Sep 9, 2015

facchinm commented Sep 10, 2015

facchinm commented Sep 10, 2015

HanaJin commented Sep 10, 2015