ESP8266: Add configurations for reset control #11331

ccli8 · 2019-08-26T05:40:40Z

Description

This PR is a replacement for #11299. By adding configurations for reset pin polarity and reset assert time in ESP8266 driver, the original reset logic _rst_pin.rst_assert()/_rst_pin.rst_deassert() can extend to support power on/off control for the ESP8266 module. This is what #11299 wants to achieve.

This PR adds ESP8266 configurations:

Add configuration for reset pin polarity
Add configuration for reset assert time

Related PR

Replacement for #11299
Rely on #11288

Pull request type

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

Reviewers

@michalpasztamobica

michalpasztamobica · 2019-08-26T05:57:17Z

components/wifi/esp8266-driver/mbed_lib.json

+            "help": "Polarity of RESET pin for the modem. [0/1]",
+            "value": 0
+        },
+        "rst-assert-time": {


What is the time unit here? I suggest to call this rst-assert-time-us or something similar and adjust to whatever unit is needed in the code, for example ThisThread::sleep_for(MBED_CONF_ESP8266_RST_ASSERT_TIME_US/100);

If this is in system ticks, we can call it rst-assert-time-ticks.
@VeijoPesonen , any thoughts on this?

I would like to know the reason why this needs to be made configurable.

@VeijoPesonen rst-pin-polarity and rst-assert-time originally are to achieve what #11299 is at. Also restate on the top.

@michalpasztamobica Change to rst-assert-time-us and adjust related code.

ThisThread::sleep_for(MBED_CONF_ESP8266_RST_ASSERT_TIME_US / 1000);

michalpasztamobica · 2019-08-26T06:09:28Z

@ccli8 , would you also like to add a target override section for M2351, so that the configs you wanted to set are automatically available without modifications to mbed_app.json?

teetak01 · 2019-08-26T06:21:56Z

components/wifi/esp8266-driver/mbed_lib.json

@@ -21,6 +21,14 @@
            "help": "RESET pin for the modem, defaults to Not Connected",
            "value": null
        },
+        "rst-pin-polarity": {
+            "help": "Polarity of RESET pin for the modem. [0/1]",
+            "value": 0


It might be useful to make use of the "options" field here to require either 0 or 1 as value here.

@teetak01 Where can I reference "options" field?

"options" example

@michalpasztamobica Thanks.
@teetak01 Use options for rst-pin-polarity.

0/1 isn't very meaningful. Have some reference to "active high" and/or "active low". CF UARTSerial::set_data_carrier_detect (although I see its doxygen comment is nonsense, sigh).

VeijoPesonen · 2019-08-26T06:34:56Z

@ccli8 The PR description should give the reason for these changes.

ciarmcom · 2019-08-26T07:00:24Z

@ccli8, thank you for your changes.
@michalpasztamobica @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

ccli8 · 2019-08-26T07:18:38Z

would you also like to add a target override section for M2351, so that the configs you wanted to set are automatically available without modifications to mbed_app.json?

@michalpasztamobica Added. But M2351 is under target renaming. This PR must rely on #11288 for M2351 new target name confirmed.

michalpasztamobica · 2019-08-26T07:24:15Z

components/wifi/esp8266-driver/mbed_lib.json

+            "rst"                   : "PD_7",
+            "rts"                   : "PD_3",
+            "cts"                   : "PD_2",
+            "rst-pin-polarity"      : "1"


Now that you didn't change the rst-assert-time-us configuration I second @VeijoPesonen 's remark - is this really necessary? If so - why not get the config for your target in here?
In general, I don't mind getting the module more configurable, but if the reset assertion time is a constant, it should stay that way.

@michalpasztamobica Add rst-assert-time-us override for M2351. Its value follows our BSP sample code.

kjbracey · 2019-08-26T07:44:00Z

Not a fan of this. Pin configurations are supposed to be program/run-time configurable.

And if there is an ESP8266 built-in to this target with custom wiring, then it should be using that run-time configuration to return an appropriately-configured ESP8266Interface object from its overriding WiFiInterface::get_default_target_instance. There shouldn't need to be any target knowledge in the ESP8266 driver itself.

kjbracey · 2019-08-26T07:54:08Z

components/wifi/esp8266-driver/mbed_lib.json

+        },
+        "rst-assert-time-us": {
+            "help": "Assert time of reset for the modem in us.",
+            "value": 2000


Why is this in microseconds, when it's really interpreted as "2 ticks", meaning "at least 1000us"? If this is going to be microseconds, then it should be set to 200, and the code should be rounding that up to 2 ticks: (us + 999) / 1000 + 1 = (us + 1999) / 1000.

But again, why is it configurable? Why would this differ between hardware? If the 3000 number for your target is correct, may as well always use it? (How many real microseconds is that supposed to represent? 2000?)

Original code is 2 ms:

ThisThread::sleep_for(2);

So I change to:

ThisThread::sleep_for(MBED_CONF_ESP8266_RST_ASSERT_TIME_US / 1000);

where MBED_CONF_ESP8266_RST_ASSERT_TIME_US defaults to 2000 us.
But it seems the code doesn't follow the doc.

Configuring MBED_CONF_ESP8266_RST_ASSERT_TIME_US to 3000 for M2351 is for power on/off control. Its discussion starts from #11299.

Right, but the point is that it isn't trying to delay for 2000us. It's trying to delay for at least 200us. If you wanted a 2000us delay, it would have to wait for 3 ticks.

Using microseconds as the unit, strongly suggests you're fishing for some sort of microsecond precision, so I would immediately question the code as written - pointing out that sleep_for(time_us / 1000) may wait for only 1000us, not the 2000us specified. (Or worse, if 1500us was requested, it could wait for 0).

If this isn't really the ESP8266's reset pin, but a module power pin, then it would be better to add a new setting for the new pin, rather than hijack the code thinking that it's driving reset. That would be in line with what we're doing for all the cellular modems - one "soft" power line which is the device's own "button", and one "hard" which is a power control.

ciarmcom · 2019-08-26T09:00:29Z

@ccli8, thank you for your changes.
@michalpasztamobica @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

ccli8 · 2019-08-27T08:28:10Z

Create #11343 to replace this one

ccli8 · 2019-08-29T09:35:25Z

Close via #11343

ccli8 mentioned this pull request Aug 26, 2019

M2351: Integrate ESP8266 power on/off logic into ESP8266 driver main… #11299

Closed

michalpasztamobica suggested changes Aug 26, 2019

View reviewed changes

teetak01 reviewed Aug 26, 2019

View reviewed changes

ccli8 added 2 commits August 26, 2019 14:42

ESP8266: Add configuration for reset pin polarity

310e583

ESP8266: Add configuration for reset assert time

0150593

ciarmcom requested review from michalpasztamobica and a team August 26, 2019 07:00

ciarmcom added the needs: review label Aug 26, 2019

ccli8 force-pushed the nuvoton_esp8266_reset_config branch from 25fd174 to bd5704b Compare August 26, 2019 07:11

michalpasztamobica reviewed Aug 26, 2019

View reviewed changes

ESP8266: Add target override for M2351

1864279

ccli8 force-pushed the nuvoton_esp8266_reset_config branch from bd5704b to 1864279 Compare August 26, 2019 07:39

kjbracey reviewed Aug 26, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Aug 26, 2019

ciarmcom requested review from michalpasztamobica and a team August 26, 2019 09:00

ciarmcom added the needs: review label Aug 26, 2019

0xc0170 removed the needs: review label Aug 26, 2019

ccli8 mentioned this pull request Aug 27, 2019

ESP8266: Support power pin in custom wiring #11343

Merged

ccli8 closed this Aug 29, 2019

michalpasztamobica mentioned this pull request Oct 9, 2019

ESP8266: Pelion connect fails with 'send tcp data' timeout #11544

Closed

cyliangtw deleted the nuvoton_esp8266_reset_config branch March 9, 2023 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESP8266: Add configurations for reset control #11331

ESP8266: Add configurations for reset control #11331

ccli8 commented Aug 26, 2019 •

edited

Loading

michalpasztamobica Aug 26, 2019

VeijoPesonen Aug 26, 2019 •

edited

Loading

ccli8 Aug 26, 2019

ccli8 Aug 26, 2019

michalpasztamobica commented Aug 26, 2019

teetak01 Aug 26, 2019

ccli8 Aug 26, 2019

michalpasztamobica Aug 26, 2019

ccli8 Aug 26, 2019

kjbracey Aug 26, 2019 •

edited

Loading

VeijoPesonen commented Aug 26, 2019

ciarmcom commented Aug 26, 2019

ccli8 commented Aug 26, 2019

michalpasztamobica Aug 26, 2019

ccli8 Aug 26, 2019

kjbracey commented Aug 26, 2019 •

edited

Loading

kjbracey Aug 26, 2019

ccli8 Aug 26, 2019

kjbracey Aug 26, 2019 •

edited

Loading

ciarmcom commented Aug 26, 2019

ccli8 commented Aug 27, 2019

ccli8 commented Aug 29, 2019

ESP8266: Add configurations for reset control #11331

ESP8266: Add configurations for reset control #11331

Conversation

ccli8 commented Aug 26, 2019 • edited Loading

Description

Related PR

Pull request type

Reviewers

Choose a reason for hiding this comment

VeijoPesonen Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalpasztamobica commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

VeijoPesonen commented Aug 26, 2019

ciarmcom commented Aug 26, 2019

ccli8 commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey commented Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

ciarmcom commented Aug 26, 2019

ccli8 commented Aug 27, 2019

ccli8 commented Aug 29, 2019

ccli8 commented Aug 26, 2019 •

edited

Loading

VeijoPesonen Aug 26, 2019 •

edited

Loading

kjbracey Aug 26, 2019 •

edited

Loading

kjbracey commented Aug 26, 2019 •

edited

Loading

kjbracey Aug 26, 2019 •

edited

Loading