-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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.
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
Conversation
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
This commit adds 80 bytes to RAM usage and some overhead in normal operations.
It's important to notice that every core based on Due should add the g_pinStatus array to its variant.cpp
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)
@HanaJin @trickedj @Tamul @DavidEGrayson @JiapengLi @billroy @3esmit , could you please test the PR? Could it be a good solution for your issues?
3esmit
commented
Jul 15, 2015
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
commented
Jul 15, 2015
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
commented
Jul 15, 2015
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
DavidEGrayson
Jul 15, 2015
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@3esmit
3esmit
Jul 15, 2015
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't checked all diffs, I'm not that deep into arduino code, but your comments makes sense. Thanks for the note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also spotted a leftover that made its way into the PR , thank you 😄 . Fixed with 5a37e94
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
DenDanny
commented
Aug 28, 2015
@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
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
commented
Sep 8, 2015
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
commented
Sep 9, 2015
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) 😉
To test the PR completely, here you can find a board-to-board test with wiring instructions on top.
HanaJin
commented
Sep 10, 2015
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
Due: solve issues with nonstandard pin operations
This PR solves the following Due broken behaviours:
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()
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 ]
8- acd trigger #1819 #2286 (#3064)
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 puredigitalWriteanddigitalRead-5% to -25%on pureanalogRead()-400%onanalogRead()followed bydigitalWrite()on different pins 😱All other cases were broken with the actual code so it's useless to test them