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.

Sign up for GitHub

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

Jump to bottom

ESP8266 unlocks deep sleep when disconnected #11514

Merged
merged 1 commit into from
Sep 27, 2019
Merged

ESP8266 unlocks deep sleep when disconnected #11514

merged 1 commit into from
Sep 27, 2019

Conversation

dmaziec
Copy link
Contributor

@dmaziec dmaziec commented Sep 18, 2019 •
edited
Loading

Description

As requested in: #10059 in ESP8266 deep sleep is locked only while the network is active or ESP8266 commands are triggered.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@AnttiKauppila
@kjbracey-arm
@michalpasztamobica

Release Notes

This release contains changes for ESP8266.
UART input is enabled and deep sleep is locked :

  • if module is connected to the network
  • for operations which communicate with the device but do not require network connectivity

UART input is disabled and deep sleep is unlocked :

  • if module is disconnected

@dmaziec dmaziec force-pushed the UART_deep_sleep_enable branch 2 times, most recently from 630eb95 to 2d1040b Compare September 18, 2019 15:21
bulislaw
bulislaw reviewed Sep 18, 2019
View reviewed changes
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

This driver changed a lot since I last looked at it so I let someone more familiar do the actual review. But the idea looks good to me.

components/wifi/esp8266-driver/ESP8266/ESP8266.h
@@ -405,6 +405,8 @@ class ESP8266 {
static const int8_t WIFIMODE_STATION_SOFTAP = 3;
static const int8_t SOCKET_COUNT = 5;

int uart_enable_input(bool lock);
Copy link
Member

Choose a reason for hiding this comment

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

Public function should have doxygen and also be bundled with rest of the methods (after flush).

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fixed now (@dmaziec1 worth adding a comment for reviewers, what has been addressed)

@ciarmcom
Copy link
Member

@dmaziec1, thank you for your changes.
@AnttiKauppila @SeppoTakalo @kjbracey-arm @michalpasztamobica @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@dmaziec dmaziec force-pushed the UART_deep_sleep_enable branch 2 times, most recently from 68a6243 to 570954c Compare September 19, 2019 07:43
@michalpasztamobica
Copy link
Contributor

@VeijoPesonen , would you also take a moment to review this?

VeijoPesonen
VeijoPesonen suggested changes Sep 20, 2019
View reviewed changes
components/wifi/esp8266-driver/ESP8266/ESP8266.h Outdated
* Enables or disables uart input and deep sleep
*
* @param lock if TRUE, uart input is enabled and deep sleep is
* locked if FALSE, uart input is disabled and deep sleep is
Copy link
Contributor

Choose a reason for hiding this comment

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

First word belongs to previous line

components/wifi/esp8266-driver/ESP8266Interface.cpp
@@ -224,13 +226,15 @@ void ESP8266Interface::_connect_async()
nsapi_error_t status = _init();
if (status != NSAPI_ERROR_OK) {
_connect_retval = status;
_esp.uart_enable_input(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling this should be done already inside _init() once it's detected that something has gone wrong with initialization. Same goes for enabling the serial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uart input enabling/disabling depends on _software_conn_stat value, so it would be better to enable or disable input once when it changes. _init should rather rely on the driver to have uart input enabled (depending on the conn_stat), than try and influence it on its own.
Also, in this way we will avoid repetition of the same condition statement before each return.
@VeijoPesonen, does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Uart input enabling/disabling depends on _software_conn_stat value, so it would be better to enable or disable input once when it changes.

Ok, point taken :)

Also, in this way we will avoid repetition of the same condition statement before each return.

Not needed based on your response.

components/wifi/esp8266-driver/ESP8266Interface.cpp

_esp.uart_enable_input(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be done already inside _power_off().

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this can stay as it is based on your previous comment about not turning the serial on inside _init().

VeijoPesonen
VeijoPesonen approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Looks good to me. I assume you have run all the netsocket test cases after doing these modifications. Those are not yet part of our PR checks.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 24, 2019

@SeppoTakalo
@AnttiKauppila
@kjbracey-arm
@michalpasztamobica

Any more reviews for this one? I'll start CI meanwhile

@mbed-ci
Copy link

mbed-ci commented Sep 24, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@michalpasztamobica
Copy link
Contributor

@VeijoPesonen , the tests have been run and passed fine (apart from a known DNS issue, we are working on anyway).

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 25, 2019

Looks like this is ready almost.

@dmaziec1 Can you add "Release notes" section in the first comment, any Functionality change should have.

@michalpasztamobica
Copy link
Contributor

@0xc0170 , release notes are added.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 26, 2019

@0xc0170 , release notes are added.

LGTM

@AnttiKauppila Please review the release notes, once approved, we will merge

@AnttiKauppila
Copy link

@ARMmbed/mbed-os-maintainers Release notes approved

@adbridge adbridge merged commit 372a3f1 into ARMmbed:master Sep 27, 2019
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
@michalpasztamobica michalpasztamobica mentioned this pull request Jun 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bulislaw bulislaw bulislaw left review comments

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

@AnttiKauppila AnttiKauppila AnttiKauppila approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@kjbracey kjbracey Awaiting requested review from kjbracey

@SeppoTakalo SeppoTakalo Awaiting requested review from SeppoTakalo

Assignees
No one assigned
Labels
release-version: 5.15.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@dmaziec @ciarmcom @michalpasztamobica @0xc0170 @mbed-ci @AnttiKauppila @bulislaw @VeijoPesonen @adbridge