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

RTC-GPIO pin calculation incorrect #822

Closed
wants to merge 2 commits into from
Closed

Conversation

husigeza
Copy link
Contributor

components/esp32/deep_sleep.c: Modifying esp_deep_sleep_get_ext1_wakeup_status so it calculates the GPIO pins from RTC pins correctly. Previously the function composed the return value incorrectly, e.g. when the returned RTC pin was Pin 0.

components/esp32/deep_sleep.c: Modifying esp_deep_sleep_get_ext1_wakeup_status so it calculates the GPIO pins from RTC pins correctly. Previously the function composed the return value incorrectly, e.g. when the returned RTC pin was Pin 0.
@CLAassistant
Copy link

CLAassistant commented Jul 20, 2017

CLA assistant check
All committers have signed the CLA.

if (!RTC_GPIO_IS_VALID_GPIO(gpio)) {
continue;
}
int rtc_pin = rtc_gpio_desc[gpio].rtc_num;
if ((status & BIT(rtc_pin)) == 0) {
continue;
}
gpio_mask |= BIT(gpio);
Copy link
Member

@igrr igrr Jul 23, 2017

Choose a reason for hiding this comment

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

Would it be sufficient to replace this line with gpio_mask |= 1ULL << gpio; to fix the issue with GPIOs higher than 32?

Copy link
Contributor Author

@husigeza husigeza Jul 24, 2017

Choose a reason for hiding this comment

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

Hello,
According to my understanding the gpio_mask should show that which PIN caused the wakeup, with setting a relevant bit to 1. E.g.: the interrupt comes through GPIO Pin3, then gpio_mask (the return value) should look like: 0x0000000000001000, because 4th bit from the right (LSB) is associated with GPIO Pin 3. However with the current solution, the gpio_mask would look like the following: 0x0000000000001100. Is this the value the function should return ? If yes then sorry, I misunderstood the concept.
Even if I misunderstood the interpretation of the gpio_mask, a problem still exists with GPIO Pin 0: gpio_mask would be 0, even the interrupt has came through Pin 0, or even no interrupt came at all.

Cheers,
Geza

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? If gpio = 0, BIT(0) = 1, and gpio_mask = 1. Are you sure you aren't misunderstanding the macro BIT? The patch does not seem like it would work. Igrr's suggestion seems right to fix gpio >= 32.

Copy link
Contributor Author

@husigeza husigeza Jul 27, 2017

Choose a reason for hiding this comment

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

Yes, you are right, sorry I did not check what BIT() does when I replied to the first comment.
Actually my change exactly does the same what Igrr suggests just with an extra, unnecessary variable.
So in summary: I agree that the only change is necessary is which proposed by Igrr.

@husigeza
Copy link
Contributor Author

The requested change is done, new commit is pushed.

@projectgus
Copy link
Contributor

Thanks @husigeza . I've squashed this commit and cherry-picked into our review & merge queue.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

LGTM

@projectgus projectgus added the Status: Pending blocked by some other factor label Aug 30, 2017
igrr pushed a commit that referenced this pull request Sep 1, 2017
@projectgus
Copy link
Contributor

Cherry-picked, thanks!

@projectgus projectgus closed this Sep 1, 2017
@igrr igrr removed the Status: Pending blocked by some other factor label Nov 14, 2017
0xFEEDC0DE64 pushed a commit to 0xFEEDC0DE64/esp-idf that referenced this pull request May 5, 2021
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.

None yet

5 participants