Skip to content

digitalWrite cleanup and more compliant with behavior on AVR #530

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

Merged
merged 3 commits into from
Jul 10, 2015
Merged

digitalWrite cleanup and more compliant with behavior on AVR #530

merged 3 commits into from
Jul 10, 2015

Conversation

h4rm0n1c
Copy link
Contributor

I rewrote digitalWrite because the existing version was breaking
functionality as compared to how it behaves on the AVR.
Specifically, I could not use digitalWrite for a library that works fine on the AVR.

Instead I had to resort to fiddling with GPOC and GPOS and bit masks,
but this rewrite made all of that unnecessary, for whatever reason it
just works better.

This version borrows a little from the AVR library in the sense that the
same logic is applied to determine whether a pin should be high or low
as the AVR version, and yes, it does appear to make a difference, at least as far as my testing on hardware has shown.

I rewrote digitalWrite because the existing version was breaking
functionality as compared to how it behaves on the AVR,. specifically, I
could not use digitalWrite for a library that works fine on the AVR.

Instead I had to resort to fiddling with GPOC and GPOS and bit masks,
but this rewrite made all of that unnecessary, for whatever reason, it
just works better.

This version borrows a little from the AVR library in the sense that the
same logic is applied to determine whether a pin should be high or low
as the AVR version, and yes, it does appear to make a difference.
@igrr
Copy link
Member

igrr commented Jul 10, 2015

The diff appears to miss an if related to pin number (==16 or not).

/Users/igrokhotkov/projects/esp8266/arduino/build/macosx/work/Arduino.app/Contents/Java/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_wiring_digital.c -o /var/folders/15/ybtbgzpj7636vv4wp92l2c700000gn/T/build6991830046946292972.tmp/core_esp8266_wiring_digital.c.o 
/Users/igrokhotkov/projects/esp8266/arduino/build/macosx/work/Arduino.app/Contents/Java/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_wiring_digital.c:91:1: error: expected identifier or '(' before '}' token
 }
 ^
Error compiling.

@igrr
Copy link
Member

igrr commented Jul 10, 2015

Could you please point to the library which caused issues? As far as I can tell, the difference between old and new code is that if someone calls digitalWrite with an even non-zero value argument (like 2 or 4), old code would interpret that as 0, hence output would be low, and in new version we will go into else branch and set output high.

Arduino documentation doesn't specify what should happen when digitalWrite is passed value which is not LOW or HIGH, so i think the library you had trouble with makes some assumptions and passes some other integers into digitalWrite.

@igrr
Copy link
Member

igrr commented Jul 10, 2015

If that is the case, the fix should be as simple as removing this line:

val &= 0x01;

Ugh, I don't know how that happened.
@h4rm0n1c
Copy link
Contributor Author

I'm sorry, I somehow managed to paste a broken function in, I could have sworn it was complete when I copied it from one file to another.

I've corrected it.

@igrr
Copy link
Member

igrr commented Jul 10, 2015

Could you please also try the original version with this line removed: val &= 0x01;?

@h4rm0n1c
Copy link
Contributor Author

I'm happy to report that you're correct, sorry if I got a little overzealous.

I've altered my pull request to reflect this, thanks.

igrr added a commit that referenced this pull request Jul 10, 2015
fix digitalWrite behavior
@igrr igrr merged commit 2487a2f into esp8266:esp8266 Jul 10, 2015
igrr added a commit that referenced this pull request Oct 29, 2015
fix digitalWrite behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants