Initial UART HW flow control support #79

VeijoPesonen · 2018-08-13T11:26:07Z

Description

Enables UART HW flow control. Requires RTS/CTS wiring if ESP8266 is on a separate module or the pins are not connected for another reason.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

Example Mbed OS app config

diff --git i/tools/test_configs/ESP8266Interface.json w/tools/test_configs/ESP8266Interface.json
index 688126511..98abe61fd 100644
--- i/tools/test_configs/ESP8266Interface.json
+++ w/tools/test_configs/ESP8266Interface.json
@@ -5,11 +5,11 @@
             "value" : "\"ESP8266Interface.h\""
         },
         "object-construction" : {
-            "value" : "new ESP8266Interface( D1, D0, false )"
+            "value" : "new ESP8266Interface()"
         },
         "connect-statement" : {
             "help" : "Must use 'net' variable name, replace WIFI_SSID, WIFI_PASSWORD, WIFI_SECURITY with your WiFi settings",
-            "value" : "((ESP8266Interface *)net)->connect(WIFI_SSID, WIFI_PASSWORD, WIFI_SECURITY)"
+            "value" : "net->wifiInterface()->connect(\"SSID\", \"PASSWORD\", NSAPI_SECURITY_WPA_WPA2)"
         },
         "echo-server-addr" : {
             "help" : "IP address of echo server",
@@ -26,12 +26,22 @@
         "tcp-client-echo-buffer-size" : {
             "help" : "Number of bytes to be send to echo server",
             "value" : "200"
+        }
+    },
+    "target_overrides": {
+        "*": {
+            "esp8266.socket-bufsize": 153600,
+            "esp8266.debug": false
         },
-        "ESP8266-TX": {
-            "value":"D1"
+        "NUCLEO_F429ZI": {
+            "esp8266.rts": "PG_12",
+            "esp8266.cts": "PG_15"
         },
-        "ESP8266-RX": {
-            "value":"D0"
+        "K64F": {
+            "esp8266.tx": "PTC4",
+            "esp8266.rx": "PTC3",
+            "esp8266.cts": "PTC2",
+            "esp8266.rts": "PTC1"
         }
     }
 }

VeijoPesonen · 2018-08-13T11:26:44Z

Status

Work in progress - would be good if someone else would also find time to test this

SeppoTakalo · 2018-08-13T11:57:07Z

ESP8266/ESP8266.cpp

+ && _parser.recv("OK\n");
+
+ _serial.set_flow_control(SerialBase::CTS, NC, _serial_cts);
+ }


Where is the else case?
When no flow control pins are enabled, should it still call something, or is the point entirely skip it?

If device have flow control set as a default, should we still disable it?

Also, on last case _serial.set_flow_contro() is called after _parser.send(), is this intentional?

I understad it so that we are not taking orders from ESP8266 when to send until ESP8266 is configured to do so.

When there are no pins configured for flow control there is no need to make any settings

kjbracey

This won't actually be 100% solid. Eg if 1 app is busy streaming data, another app may be making read calls for its socket, despite the fact that the first app hasn't consumed its data. In that scenario we will be filling up local buffers with socket 1 data to deliver the socket 2 data.

Still, better than nothing.

kjbracey · 2018-08-13T11:40:58Z

ESP8266/ESP8266.cpp

 free(q);
+ _heap_usage -= sizeof(struct packet) + alloc_len;


Formatting - tabs.

kjbracey · 2018-08-13T11:41:30Z

ESP8266/ESP8266.h

@@ -304,6 +316,8 @@ class ESP8266
 int _socket_open[SOCKET_COUNT];
 nsapi_connection_status_t _connection_status;
 Callback<void(nsapi_event_t, intptr_t)> _connection_status_cb;
+ static const int32_t _MAX_HEAP_USAGE = MBED_CONF_ESP8266_SOCKET_BUFSIZE;


Sizes should probably be size_t throughout.

kjbracey · 2018-08-13T11:42:27Z

ESP8266/ESP8266.cpp

- _packets(0), 
+// Big enough, target specific
+#ifndef MBED_CONF_ESP8266_SOCKET_BUFSIZE
+#define MBED_CONF_ESP8266_SOCKET_BUFSIZE 102400


If you have a default in the JSON, don't need a default here. (Gets confusing when people try hacking around)

True, I'll take this away and leave it to mbed_lib.json

kjbracey · 2018-08-13T11:46:10Z

ESP8266/ESP8266.cpp

+ _serial.set_flow_control(SerialBase::RTSCTS, _serial_rts, _serial_cts);
+
+ // Start ESP8266's flow control
+ done = _parser.send("AT+UART_CUR=%u,%u,%u,%u,%u", ESP8266_DEFAULT_BAUD_RATE, DATABITS_8, STOPBITS_1,


If you're going to the trouble of using all those %u to parametrise, then why not just have a single send call that takes the mode as a parameter, rather than putting it in 3 places?

Personally wouldn't object to just putting 8,1,0 in there literally, rather than spending the code size on the %us.

Personally wouldn't object to just putting 8,1,0 in there literally, rather than spending the code size on the %us.

I agree

kjbracey · 2018-08-13T11:48:43Z

ESP8266/ESP8266.cpp

 packet->next = 0;

 if (_parser.read((char*)(packet + 1), amount) < amount) {
 free(packet);
+ _heap_usage -= pdu_len;
+ MBED_ASSERT(_heap_usage >= 0);


... Although if you did change to size_t, this would need to change to an assert that pdu_len >= _heap_usage before subtraction.

kjbracey · 2018-08-13T12:03:32Z

ESP8266/ESP8266.h

@@ -20,6 +20,7 @@
 #include "ATCmdParser.h"
 #include "nsapi_types.h"
 #include "rtos.h"
+#include "UARTSerial.h"


Don't see reason for added include here.

SeppoTakalo · 2018-08-13T12:01:40Z

ESP8266/ESP8266.cpp

+
+ if ((_heap_usage + pdu_len) > _MAX_HEAP_USAGE) {
+ debug("ESP8266: socket buffer max memory limit exceeded, either increase socket buffer size from mbed_lib.json or enable UART HW flow control\n");
+ MBED_ASSERT(false);


Use MBED_ERROR instead. (Ask Kari for help)

SeppoTakalo · 2018-08-13T12:04:57Z

ESP8266/ESP8266.h

+ *
+ * @return true if started
+ */
+ bool start_uart_hw_flow_ctrl();


Should this be private?
We are calling this, and not expecting users to call it? am I right?

Called from ESP8266Interface.cpp so needs to be public

VeijoPesonen · 2018-08-13T12:07:15Z

Required RTS/CTS wiring

Bootloader wiring

Be aware that the ESP8266's RTS pin is GPIO15 which is also used for boot mode selection

GPIO0 on ESPBee

Pull down for firmware update - pull up for flash boot

References

ESPBee Pinout
ESP8266 Bootloader Modes and GPIO state on Startup
ESP8266 AT Instruction Set
Trouble Flashing Your ESP8266? Meet DIO and QIO
NUCLEO-F429ZI board pinout

SeppoTakalo · 2018-08-13T12:10:58Z

Since you already have pictures, can you add section to README.md of how to enable the flow control? And add those pictures there. (Also, rotate pictures so that text is vertical)

VeijoPesonen · 2018-08-20T08:49:02Z

Squashed and rebased

VeijoPesonen · 2018-08-20T13:05:50Z

@marcuschangarm I fixed a stupid mistake which prevented flow control working at all. I had configured the flow control before running reset for the ESP8266.. Please update

VeijoPesonen · 2018-08-21T10:48:36Z

README.md updated to have an example how to enable UART HW flow control

SeppoTakalo

On the previous section The serial port does not have hardware flow control enabled.
change this to does not have hardware flow control enabled by default

Then add a note that not all modules expose RTS/CTS pins.

…ontrol

… flow control

VeijoPesonen added the do not merge label Aug 13, 2018

VeijoPesonen requested review from SeppoTakalo, marcuschangarm, kjbracey, mikaleppanen and kseverinkangas-zg August 13, 2018 11:30

SeppoTakalo reviewed Aug 13, 2018

View reviewed changes

kjbracey reviewed Aug 13, 2018

View reviewed changes

SeppoTakalo reviewed Aug 13, 2018

View reviewed changes

VeijoPesonen force-pushed the feature-hw_flow_control branch from 4616b77 to 62bd7c1 Compare August 16, 2018 10:23

Initial UART HW flow control support

28552b8

VeijoPesonen force-pushed the feature-hw_flow_control branch from 62bd7c1 to 28552b8 Compare August 20, 2018 08:48

fixup! Initial UART HW flow control support

eb3deb0

SeppoTakalo suggested changes Aug 21, 2018

View reviewed changes

Updates README.md to include information how to enable UART HW flow c…

5130604

…ontrol

VeijoPesonen force-pushed the feature-hw_flow_control branch from 3598917 to 5130604 Compare August 21, 2018 11:01

Veijo Pesonen added 2 commits August 21, 2018 15:16

fixup! Initial UART HW flow control support

defe6ec

fixup! Updates README.md to include information how to enable UART HW…

6ecebb7

… flow control

SeppoTakalo approved these changes Aug 21, 2018

View reviewed changes

VeijoPesonen removed the do not merge label Aug 21, 2018

VeijoPesonen merged commit 7bb1d8e into master Aug 21, 2018

VeijoPesonen deleted the feature-hw_flow_control branch August 22, 2018 04:24

VeijoPesonen mentioned this pull request Sep 14, 2018

Feature tcp passive mode with AT firmware v1.7.0 #83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial UART HW flow control support #79

Initial UART HW flow control support #79

VeijoPesonen commented Aug 13, 2018 •

edited

Loading

VeijoPesonen commented Aug 13, 2018

SeppoTakalo Aug 13, 2018

SeppoTakalo Aug 13, 2018

VeijoPesonen Aug 13, 2018

VeijoPesonen Aug 13, 2018

kjbracey left a comment

kjbracey Aug 13, 2018

kjbracey Aug 13, 2018

kjbracey Aug 13, 2018

VeijoPesonen Aug 13, 2018

kjbracey Aug 13, 2018

VeijoPesonen Aug 13, 2018 •

edited

Loading

kjbracey Aug 13, 2018

kjbracey Aug 13, 2018

SeppoTakalo Aug 13, 2018

SeppoTakalo Aug 13, 2018

VeijoPesonen Aug 13, 2018

VeijoPesonen commented Aug 13, 2018

SeppoTakalo commented Aug 13, 2018

VeijoPesonen commented Aug 20, 2018

VeijoPesonen commented Aug 20, 2018

VeijoPesonen commented Aug 21, 2018

SeppoTakalo left a comment

Initial UART HW flow control support #79

Initial UART HW flow control support #79

Conversation

VeijoPesonen commented Aug 13, 2018 • edited Loading

Description

Pull request type

Example Mbed OS app config

VeijoPesonen commented Aug 13, 2018

Status

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 left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeijoPesonen Aug 13, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeijoPesonen commented Aug 13, 2018

Required RTS/CTS wiring

Bootloader wiring

GPIO0 on ESPBee

References

SeppoTakalo commented Aug 13, 2018

VeijoPesonen commented Aug 20, 2018

VeijoPesonen commented Aug 20, 2018

VeijoPesonen commented Aug 21, 2018

SeppoTakalo left a comment

Choose a reason for hiding this comment

VeijoPesonen commented Aug 13, 2018 •

edited

Loading

VeijoPesonen Aug 13, 2018 •

edited

Loading