Skip to content
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

Fix for wrong GPIO reg type (issue #397) #398

Closed

Conversation

tprochazka
Copy link

I think that is should be like this. (issue #397)

@tprochazka tprochazka force-pushed the issues/397-gpio-reg-type branch from dd578e6 to 8bfca93 Compare November 3, 2015 23:24
@tprochazka
Copy link
Author

I would appreciate some feedback ;-)

#define portOutputRegister(P) ( ((volatile uint8_t*)(P != PC ? STD_GPIO_OUT : RTC_GPIO_OUT)) + ( ( ((int)P) == PB ) ? 1 : 0) )
#define portInputRegister(P) ( ((volatile uint8_t*)(P != PC ? STD_GPIO_IN : RTC_GPIO_IN_DATA)) + ( ( ((int)P) == PB ) ? 1 : 0) )
#define portModeRegister(P) ( ((volatile uint8_t*)(P != PC ? STD_GPIO_ENABLE : RTC_GPIO_ENABLE)) + ( ( ((int)P) == PB ) ? 1 : 0) ) // Stored bits: 0=In, 1=Out
#define portOutputRegister(P) ( ((GPIO_REG_TYPE*)(P != PC ? STD_GPIO_OUT : RTC_GPIO_OUT)) + ( ( ((int)P) == PB ) ? 1 : 0) )
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what it's right correction. (uint32_t* + 1) will go out of register range.

Copy link
Author

Choose a reason for hiding this comment

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

I'm also not sure. I just changed uint8_t to uint32_t according Arduino/ESP8266 project to solve my problem #397 which works.

@anakod
Copy link
Member

anakod commented Nov 9, 2015

Originaly in Sming I have used "pseudo" sub-registers with type uint8_t* (wich cover part of original full 32-bit ESP register) to be compatible with large amount of old Arduino libraries. On other side now we have many ported libraries, and we can switch to incompatible with old Arduino 32bit register, but it need more detailed checking and refactoring. Did we have reason for that? What will be better way for end users?

@tprochazka
Copy link
Author

The reason for that is described here
#397
Masking operation used during SPI communication change also GPIO9 and GPIO10, probably because these pins are outside your original 8 bit register.

Best way how reproduce is use SSD1331 example and add for example blink code for GPIO9 and 10.

@robotiko
Copy link

@tprochazka
do you think that this also will fix the direct crash in esp-12e (nodemcu devkit 1.0) when trying to set the pin?
Simply
pinMode(9, OUTPUT);
digitalWrite(9, 0);)

makes a nice reset

@hreintke
Copy link
Contributor

@tprochazka : @robotiko :
Can you update with status ?
If there needs to be a discussion on " "pseudo" sub-registers" before I suggest to close this PR and open an issue for further communication.

@robotiko
Copy link

@hreintke
I'm waiting for @tprochazka reply to check in related issues that are affected by this.
So far, no news.

@tprochazka
Copy link
Author

@robotiko Sorry for delay. I don't think that it is related. If you check digitalWrite() implementation it already using uint32 as register if I understand it correctly.

@alonewolfx2
Copy link
Member

gpio9 issue flash chip related. if you cut gpio9 to flash connection you can use gpio9. tested,confirmed.

@tprochazka
Copy link
Author

I have no problem with using GPIOP9 on Nodemcu 1.0 without any modification.

@alon24
Copy link

alon24 commented Dec 6, 2015

is your nodemcu amica brand, do u flash dio, and if so how, can u attach
rooms here, do u use linux, Mac, or what. you r the only one who says
this works

On Sun, Dec 6, 2015, 19:48 Tomáš Procházka [email protected] wrote:

I have no problem with using GPIOP9 on Nodemcu 1.0 without any
modification.


Reply to this email directly or view it on GitHub
#398 (comment).

@robotiko
Copy link

robotiko commented Dec 6, 2015

@tprochazka
I' m using a couple of amica nodemcu devkit 1.0 without success.
Sming Dev code with SDK 1.4 on windows, setting the DIO mode in rboot and non rboot code examples.
GPIO10 working without issues.. GPIO9 .. not working at all .. always reset when setting output.
We recently found that DIO setting was having no effect while flashing ( this bug recently just got fixed)

I read in many forums.. (many people having the same issue), that Amica modules should work due to the flash module they use, but other ESP12e.. depend on flash chip.
So there is no reason why we are having this issue (amica users).
Can you provide a code example or binary to test? so we can know if it is HW related or SW related issue?

As @alon24 pointed, What is you environment? SDK, SO, Sming version?

Thanks a lot.

@zhivko
Copy link

zhivko commented Dec 7, 2015

I use nodekit 1.0 devkit12E from aliexpress:
http://www.aliexpress.com/item/10-PCS-New-Wireless-module-4M-4FLASH-NodeMcu-Lua-WIFI-Networking-development-board-Based-ESP8266/32454268280.html
And tried GPIO write with develop branch SMING, and sdk 1.5.
Result is as soon as gpio is marked for output:
pinMode(9, OUTPUT);
module freezes and then wdt reset happens.

@zhivko
Copy link

zhivko commented Dec 7, 2015

According to this: http://bbs.espressif.com/viewtopic.php?t=654, the gpio9 and gpio10 must not be physically connected to flash.
Since we can do QIO flashing on nodemcu esp12e dev 1.0 it means GPIO9 and GPIO10 are connected to flash - and there fore we could not use those two pins??

@alon24
Copy link

alon24 commented Dec 7, 2015

can someone run that code on devkit 1.0?
And see if this is the truth (need to flash in dio mode ofcourse)

On Mon, Dec 7, 2015 at 2:57 PM, zhivko [email protected] wrote:

According to this: http://bbs.espressif.com/viewtopic.php?t=654, the
gpio9 and gpio10 must not be physically connected to flash.
Since we can do qio on nodemcu esp12e dev 1.0 it means GPIO9 and GPIO10
are connected to flash - and there fore we could not use those two pins??


Reply to this email directly or view it on GitHub
#398 (comment).

@alonewolfx2
Copy link
Member

the gpio9 and gpio10 must not be physically connected to flash.

as i said before.

@zhivko
Copy link

zhivko commented Dec 7, 2015

I did a test on nodemcu and flash in dio mode and was NOT able to use gpio9.
On Dec 7, 2015 5:32 PM, "alonewolfx2" [email protected] wrote:

the gpio9 and gpio10 must not be physically connected to flash.

as i said before.


Reply to this email directly or view it on GitHub
#398 (comment).

@alonewolfx2
Copy link
Member

I did a test on nodemcu and flash in dio mode and was NOT able to use gpio9.

without cutting gpio9 to flash chip connection right ?

@zhivko
Copy link

zhivko commented Dec 7, 2015

Exactly.
On Dec 7, 2015 6:01 PM, "alonewolfx2" [email protected] wrote:

I did a test on nodemcu and flash in dio mode and was NOT able to use
gpio9.

without cutting gpio9 to flash chip connection right ?


Reply to this email directly or view it on GitHub
#398 (comment).

@robotiko
Copy link

robotiko commented Dec 7, 2015

Hi, since the GIPIO9 issue doesn't seem to be related to this issue, please continue side conversation in the #474

@hreintke
Copy link
Contributor

@ALL :
Can you clarify the status of the PR. ?

@hreintke hreintke added the Bug label Dec 21, 2015
@hreintke
Copy link
Contributor

@tprochazka @robotiko @alonewolfx2

I will probably release a bugfix V2.1 release shortly and would like to have this included.
Can you finalize is PR and confirm correctness ?

@tprochazka
Copy link
Author

Is any action needed from my side?

@robotiko
Copy link

@hreintke my GPIOP9 issues looks like it is not directly related to this issue and moved to #474.
I had no additional involvement with this issue. Didn't test.

@tprochazka
Copy link
Author

Is there still something preventing this from merge?
Or just fix this problem in better way, that I did and close this PR?

@hreintke
Copy link
Contributor

@zardam : @tprochazka :
I don't how this does or does not interfere with SmingHub/SmingRTOS#70

I prefer to have one PR which can be included in both NONOS and RTOS

@hreintke
Copy link
Contributor

Outdated

@hreintke hreintke closed this May 23, 2016
@hreintke hreintke removed the Review label May 23, 2016
@tprochazka
Copy link
Author

I don't understand why this was fixed in RTOS version and not here, id this is still under development, or not?

@slaff slaff reopened this Jul 15, 2017
@slaff
Copy link
Contributor

slaff commented Jul 15, 2017

@tprochazka Can you check if this PR is still working with the latest develop version?

@tprochazka
Copy link
Author

Fix itself is still working, but there are now some more examples which expecting bad variable type.
All the libraries and examples should use defined macro GPIO_REG_TYPE instead of volatile uint8_t*

This is the biggest problem of Sming. I very like it's API and whole concept, but the Wire implementation is very outdated and there are too differences against Arduino which make very difficult to use existing Arduino libraries. This is the reason why I'm now using Arduino ESP8266 implementation instead of sming. Maybe best way would be use some base code directly from the Arduino ESP 8266 and make the perfect API for working with HTTP, FTP, etc on top of it.

@slaff
Copy link
Contributor

slaff commented Jul 20, 2017

Fix itself is still working, but there are now some more examples which expecting bad variable type.

Can you rebase your PR based on the latest develop and then fix all examples that are affected?

but the Wire implementation is very outdated and there are too differences against Arduino

The Wire implementation was updated recently (0c2d053) and PR #1193, that will be merged after 3.3.0 is released, takes care to decrease the differences with Arduino

@slaff slaff mentioned this pull request Jul 21, 2017
10 tasks
@slaff slaff added this to the 3.4.0 milestone Jul 21, 2017
@tprochazka tprochazka force-pushed the issues/397-gpio-reg-type branch from 8bfca93 to c8eda92 Compare September 15, 2017 21:59
@tprochazka tprochazka force-pushed the issues/397-gpio-reg-type branch from c8eda92 to 8e02e2b Compare September 15, 2017 22:12
@tprochazka
Copy link
Author

I updated my pull request, there was just one more place where was necessary to use macro GPIO_REG_TYPE. But I did not test it on the real device.

#define portInputRegister(P) ( ((volatile uint8_t*)(P != PC ? STD_GPIO_IN : RTC_GPIO_IN_DATA)) + ( ( ((int)P) == PB ) ? 1 : 0) )
#define portModeRegister(P) ( ((volatile uint8_t*)(P != PC ? STD_GPIO_ENABLE : RTC_GPIO_ENABLE)) + ( ( ((int)P) == PB ) ? 1 : 0) ) // Stored bits: 0=In, 1=Out
#define portOutputRegister(P) ( ((GPIO_REG_TYPE*)(P != PC ? STD_GPIO_OUT : RTC_GPIO_OUT)) + ( ( ((int)P) == PB ) ? 1 : 0) )
#define portInputRegister(P) ( ((GPIO_REG_TYPE*)(P != PC ? STD_GPIO_IN : RTC_GPIO_IN_DATA)) + ( ( ((int)P) == PB ) ? 1 : 0) )
Copy link
Contributor

Choose a reason for hiding this comment

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

volatile uint8_t* was changed to GPIO_REG_TYPE* which is wrong. Without the volatile keyword the compiler may optimize-out the code. Please, bring back the volatile keyword.

@slaff slaff closed this in #1213 Oct 14, 2017
@slaff
Copy link
Contributor

slaff commented Oct 14, 2017

@tprochazka Can you get the latest develop version and tell me if the issue that you had is fixed for you?

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.

8 participants