esp8266/uart: rework uart io structure so it can work with dupterm() and be used to disable stdio #2891

mhoffma · 2017-02-21T01:02:08Z

This is the start in a series of refactorings of the UART on esp8266 so we can move in the direction of a single slotted. The first in the series of commits will globalize the UART py structure so that we have direct access to the py object from the root state structure. The original dialogue #2814 which discusses a hack implementation that allowed for disabling stdio on the UART for boards which only have a single serial port.

mhoffma · 2017-02-21T03:44:36Z

How does the group feel about moving input_buf into the py UART structure, the ask is if its reasonable direction to take

typedef struct _pyb_uart_obj_t {
    mp_obj_base_t base;
    uint8_t uart_id;
    uint8_t bits;
    uint8_t parity;
    uint8_t stop;
    uint32_t baudrate;
    uint16_t timeout;       // timeout waiting for first char (in ms)
    uint16_t timeout_char;  // timeout waiting between chars (in ms)
    ringbuf_t input_buf;
    byte input_buf_array[256];
} pyb_uart_obj_t;

dpgeorge · 2017-02-21T07:30:03Z

How does the group feel about moving input_buf into the py UART structure,

The input buffer for the REPL should be separate from the UART (so webrepl can work without uart). The uart itself can have an rx buffer, but that should ideally be dynamically allocated so memory is not wasted if the buffer is not used. And I think the esp8266 has a large-ish internal uart buffer so one might get away without a uart rx buffer at all.

…wait functionality

mhoffma

using the hardware structure for uart_rx_any doesn't actually work, we have pulled the samples out by the time we get here. I think input_buf actually needs to be part of the low-level uart code and not spread out.

mhoffma · 2017-02-22T13:13:29Z

couple of subtle points not to get lost:

I believe the uart needs to be separated out, so we can use it with gdbstub and mp.
The uart requires an additional buffer because if we don't drain the RX buf in the ISR the interrupt will keep tripping, this is an observation I made during the above sequence of changes.
The first attempt at nostdio didn't actually work so its good you @dpgeorge pointed out that it was hacky everything was being multiplexed into that input_buf both streams and it was just coincidental that it appeared to work.

So I think REPL needs an input_buf, with its signaling functions that input has arrived using of course the same naming scheme. And the uart or what ever else needs to be responsible for buffering its own input and causing the async signaling to occur.

mhoffma · 2017-02-23T05:34:11Z

picture is work in progress

with this series of patches the following behavior exists. Current design works as is functionality new functionality is predicated on term_obj being set via dupterm. Once the dupterm has been configured the serial port is disabled and can be provisioned to other functions! nostdio over uart folded into the dupterm infrastructure.

connecting uart to repl via dupterm works as expected.

>>> import os
>>> os.dupterm()
>>> import machine
>>> u=machine.UART(0)
>>> u
UART(0, baudrate=115200, bits=8, parity=None, stop=1, timeout=0, timeout_char=1)
>>> os.dupterm(u)
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> os.dupterm()
UART(0, baudrate=115200, bits=8, parity=None, stop=1, timeout=0, timeout_char=1)
>>>

I tested webrepl and my little telnet server and things seem to function well reading and writing files etc. I'm going to add another field to UART for the buffer as suggested and make the default 16 for UART0 and 0 for UART1 which is only valid for TX anyways.

mhoffma · 2017-02-23T05:50:06Z

svily0 · 2017-04-04T08:48:51Z

Well, I actually started from here.
The board still reboots when data from my acquisition device contains 0x03 in it. Hence the patch.

Revert part of previus commit

mohpor · 2017-04-18T07:07:06Z

@mhoffma
Hi, Thanks for this PR.
I'm very much interested in the implementation of this feature.
That being said, I don't have enough knowledge nor time (at the moment) to help you further this cause, but I defintely need this feature in the near future and I can help test this.
Now, I have successfully deployed your fork onto my ESP-12F, everything appears to be ok but I have a problem which appears to be related to UART's buffer. When I try to read from UART, I can't get the full extent of expected values, but only 15 characters. To clear my comment up, I'm using the UART to send AT comands to another module , I know the expected responses my other module would send (through both documentation and direct read from its UART in a separate config). When I send AT\r\n I can successfully read OK\r, no problems, but when I send another AT command which potentially returns a value of multiple lines, I can read only 15 characters! it seems like UART doesn't buffer more than that.

Is my conclusion coreect?
Am I doing something wrong?
How can I fix this situation?

P.S.: Using main branch of the upstream Micropython repo, when I send my AT commands, I can see the full response getting back from my module (which will blow back to my REPL and causes mass confusion!)

craftyguy · 2017-05-20T06:49:49Z

@mohpor

I suspect this part of the patch is causing you to only receive 15 characters (technically, 15 printables + a \0 ?)

if (self->uart_id == 0 && self->rxbuflen < 16)
    self->rxbuflen=16;        /* probably too restrictive */

I don't know why @mhoffma is so restrictive in selecting a buffer of this size.. perhaps he will inform us!

craftyguy · 2017-05-20T07:09:46Z

@mohpor
So, after a little more inspection, it seems like you can define a rxbuflen parameter when initializing the machine.UART object. If you don't, it defaults to 16 characters.

craftyguy · 2017-05-20T07:14:55Z

@dpgeorge What open concerns do you have with this PR that are preventing it from being merged? Not OP, but definitely interested in this!

dpgeorge · 2017-05-21T03:02:07Z

What open concerns do you have with this PR that are preventing it from being merged?

In general it looks OK, but since it makes some deep changes wrt UART processing it needs a thorough review. It will need to wait until after v1.9. There are also some code style issues, such as missing curly braces for single-line statement blocks, and missing spaces around operators, like =.

dpgeorge · 2017-06-01T07:39:14Z

@mhoffma are you still interested in working on this? As said above there are lots of code style problems. If you don't have time then I will take it from here.

mhoffma · 2017-06-01T15:13:43Z

I am interested in this, but my availability has dwindled. I got a little time this weekend to merge to the latest and see that it all still functions well and was planing to push that to my pull request unless you don't want me to. Please help me isolate the changes you require and I will make time this weekend I have a few hours in the early morning that I can contribute. Maybe there is a special git diff that can help, if there is I'm not sure how to use it. Thanks Marc

…

On Thu, Jun 1, 2017 at 3:39 AM, Damien George ***@***.***> wrote: @mhoffma <https://github.com/mhoffma> are you still interested in working on this? As said above there are lots of code style problems. If you don't have time then I will take it from here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQEAU4Bb_e43TMVGv3Xe5opIoHt-6N-Wks5r_mqvgaJpZM4MGw0o> .

mhoffma · 2017-06-02T02:14:01Z

esp8266/esp_mphal.c

+void mp_hal_uart_rx_intr(int uart_no) {
+    int ch;
+    mp_obj_t term = MP_STATE_PORT(term_obj);
+    bool uart_term=(term == NULL || term == MP_STATE_PORT(pyb_uart_objs)[uart_no]);


should this be

bool uart_term = (term == NULL || term == MP_STATE_PORT(pyb_uart_objs)[uart_no]);

mhoffma · 2017-06-02T02:14:48Z

esp8266/esp_mphal.c

+    bool uart_term=(term == NULL || term == MP_STATE_PORT(pyb_uart_objs)[uart_no]);
+
+    for (;;) {
+        if ((ch = uart_rx_one_char(uart_no))==-1)


is this the correct spacing?

if ((ch = uart_rx_one_char(uart_no)) == -1)

dpgeorge · 2017-06-02T08:16:01Z

esp8266/esp_mphal.c

@@ -146,6 +149,31 @@ void mp_hal_signal_input(void) {
    #endif
 }

+void mp_hal_uart_rx_intr(int uart_no) {


IMO this belongs in machine_uart.c, since it's specific to UART behaviour. Also, since UART1 can't receive it doesn't need to take an argument.

dpgeorge · 2017-06-02T08:16:23Z

esp8266/esp_mphal.c

+            if (ch == mp_interrupt_char)
+                mp_keyboard_interrupt();
+            else
+                ringbuf_put(&input_buf, ch);


Please use curly braces for single-line bodies.

dpgeorge · 2017-06-02T08:16:55Z

esp8266/esp_mphal.h

@@ -40,6 +40,10 @@ extern const struct _mp_print_t mp_debug_print;
 extern ringbuf_t input_buf;
 // Call this after putting data to input_buf
 void mp_hal_signal_input(void);
+// Call this to put characters into connected uart buffer if repl connected
+void mp_hal_uart_rx_intr(int uart_no);


The way this function is currently implemented, it handles chars even if REPL is not connected.

dpgeorge · 2017-06-02T08:17:14Z

esp8266/machine_uart.c

 #include "modmachine.h"

 // UartDev is defined and initialized in rom code.
 extern UartDevice UartDev;

+#define RXBUF_SIZE 16


This is unused.

dpgeorge · 2017-06-02T08:17:40Z

esp8266/machine_uart.c

@@ -73,6 +89,7 @@ STATIC void pyb_uart_init_helper(pyb_uart_obj_t *self, size_t n_args, const mp_o
        //{ MP_QSTR_rx, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
        { MP_QSTR_timeout, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 0} },
        { MP_QSTR_timeout_char, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 0} },
+        { MP_QSTR_rxbuflen, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 0} },


Use 16 instead of 0 for the default.

dpgeorge · 2017-06-02T08:17:55Z

esp8266/machine_uart.c

@@ -155,6 +172,10 @@ STATIC void pyb_uart_init_helper(pyb_uart_obj_t *self, size_t n_args, const mp_o
        self->timeout_char = min_timeout_char;
    }

+    self->rxbuflen = args[ARG_rxbuflen].u_int;
+    if (self->uart_id == 0 && self->rxbuflen < 16) {


Don't need this check if 16 is given as a default above.

dpgeorge · 2017-06-02T08:18:55Z

esp8266/machine_uart.c

@@ -211,6 +244,29 @@ STATIC const mp_rom_map_elem_t pyb_uart_locals_dict_table[] = {

 STATIC MP_DEFINE_CONST_DICT(pyb_uart_locals_dict, pyb_uart_locals_dict_table);

+
+


Please don't add blank lines like this. Follow existing conventions.

dpgeorge · 2017-06-02T08:19:44Z

esp8266/machine_uart.c

    pyb_uart_init_helper(self, n_args - 1, args + 1, &kw_args);

+    if (self->rxbuflen) {
+      if (self->buf) {


Please use 4 spaces for indenting.

dpgeorge · 2017-06-02T08:33:49Z

To move forward with this we really need to decide if the dupterm (on esp8266) will have only 1 slot, or more than one. This PR goes on the basis of 1 slot only. That will mean that (eventually, doesn't need to be address right now by this PR):

uos.dupterm(machine.UART(0, 115200)) should be added to boot.py so the REPL appears by default on the UART.
uos.dupterm(None) can be used to completely disable the REPL. For example, this is used in cases where the UART is freed for other use and there's no WebREPL (normal case for a stand-alone device doing its thing). In particular this means that a NULL dupterm object does not mean to use the default UART, as this PR currently assumes.
webrepl needs to be modified so that it restores the UART once it is closed/deactivated. But it should only restore it if the UART was there before webrepl was activated.

On the other hand, if dupterm has 2 slots then it can behave as:

uos.dupterm(machine.UART(0, 115200), 1) can be added to boot.py to put UART in slot 1 (the second slot).
uos.dupterm(None, 1) can be used to disable UART.
webrepl would do uos.dupterm(ws) to enable itself, and uos.dupterm(None) to disable, which would work by having slot 0 as the default (equivalently one could write uos.dupterm(ws, 0) and uos.dupterm(None, 0)). webrepl doesn't need to know if UART is enabled or not.

For ports that want multiple dupterm slots it would be good to specify the above behaviour (or something similar) in the docs. For esp8266 it's then a question of whether it has the resources to implement 2 slots.

mhoffma · 2017-06-02T14:25:40Z

I'm confused on multiple slots (2), I think you can build that behavior right in Python keeping the kernel stuff as simple as possible. With python you can insert any object that looks like a stream into the terminal slot and have it do what ever you want lets say logging to the flash at the same time as redirecting to webrepl and to a telnet session and of course the uart (we can do what ever we want with the python language). Having that additional state seems ok but I don't get what it really buys us when you can model the behavior in python already. What am I missing?

…

On Fri, Jun 2, 2017 at 4:34 AM, Damien George ***@***.***> wrote: To move forward with this we really need to decide if the dupterm (on esp8266) will have only 1 slot, or more than one. This PR goes on the basis of 1 slot only. That will mean that (eventually, doesn't need to be address right now by this PR): - uos.dupterm(machine.UART(0, 115200)) should be added to boot.py so the REPL appears by default on the UART. - uos.dupterm(None) can be used to completely disable the REPL. For example, this is used in cases where the UART is freed for other use and there's no WebREPL (normal case for a stand-alone device doing its thing). In particular this means that a NULL dupterm object does not mean to use the default UART, as this PR currently assumes. - webrepl needs to be modified so that it restores the UART once it is closed/deactivated. But it should only restore it if the UART was there before webrepl was activated. On the other hand, if dupterm has 2 slots then it can behave as: - uos.dupterm(machine.UART(0, 115200), 1) can be added to boot.py to put UART in slot 1 (the second slot). - uos.dupterm(None, 1) can be used to disable UART. - webrepl would do uos.dupterm(ws) to enable itself, and uos.dupterm(None) to disable, which would work by having slot 0 as the default (equivalently one could write uos.dupterm(ws, 0) and uos.dupterm(None, 0)). webrepl doesn't need to know if UART is enabled or not. For ports that want multiple dupterm slots it would be good to specify the above behaviour (or something similar) in the docs. For esp8266 it's then a question of whether it has the resources to implement 2 slots. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQEAU0kjEqEeA_tOQ0cKS1gTKUKyeGi_ks5r_8kGgaJpZM4MGw0o> .

mhoffma · 2017-06-02T14:36:48Z

Back on Feb13 we talked about the multiple slot stuff with @pfalcon on thread #2814 I think we came to the suggestion of keep things simple.

pfalcon · 2017-06-03T11:43:48Z

uos.dupterm(machine.UART(0, 115200)) should be added to boot.py so the REPL appears by default on the UART.

Yes, that's how it's intended to be. (Then people will throw away that line and cry "where's my repl???777".)

On the other hand, if dupterm has 2 slots then it can behave as

Yes, I guess let's do it that way. My concern, as before, is that this "explicit slot numbers" are somewhat adhoc. The situation is as with not allowing to mount a filesystem on root. But mounting is very user-visible feature, and for dupterm(), I guess we can go for compromise.

pfalcon · 2017-06-03T11:47:03Z

I'm confused on multiple slots (2), I think you can build that behavior right in Python keeping the kernel stuff as simple as possible.

I'm not sure what exactly you mean, but we discussed that in the ticket you linked to. Summary: a) @dpgeorge wants to provide "duplicate", not "switch" behavior out of the box; b) it's one thing to be able to potentially implement in Python, another to make it actually work reliably (with low memory, etc.).

mhoffma · 2017-06-07T00:59:36Z

That's fine we can do it anyway you want of course. Is it possible that we add that behavior on a separate PR, adding this behavior in two stages along the lines of an incremental fashion?

@dpgeorge

From @dpgeorge on micropython#2891: IMO this belongs in machine_uart.c, since it's specific to UART behaviour. Also, since UART1 can't receive it doesn't need to take an argument. Makes more sense here as it is UART specific.

dpgeorge · 2018-05-21T01:34:31Z

Ability to disbale UART(0) on the REPL was implemented in afd0701

esp8266/uart: hardwire references to the uarts in ROOT

941568b

mhoffma closed this Feb 21, 2017

mhoffma reopened this Feb 21, 2017

mhoffma added 3 commits February 21, 2017 08:29

esp8266/uart_poll: remove input_buf depend use RX buffer CNT

293f40a

esp8266/uart: uart_rx_wait now use HW uart RXFIFO_CNT and STATUS for …

d31d4b6

…wait functionality

esp8266/uart: bottom half rx to call handler function to drain samples

2cb4b5c

mhoffma commented Feb 22, 2017

View reviewed changes

esp8266/uart: support uart redirection via dupterm

3753d28

mhoffma added 2 commits February 23, 2017 01:19

esp8266/uart: dynamic buffer allocation for rxbuf via rxbuflen

a9c8f8d

esp8266/uart: white space

1916cef

mhoffma changed the title ~~esp8266/uart: hardwire references to the uarts in ROOT~~ esp8266/uart: rework uart io structure so it can work with dupterm() and be used to disable stdio Feb 23, 2017

dpgeorge mentioned this pull request Apr 4, 2017

Esp8266 uart fixes #2990

Closed

svily0 referenced this pull request in svily0/micropython Apr 5, 2017

Disable REPL's input interpretation

da8b640

Revert part of previus commit

dpgeorge mentioned this pull request Apr 10, 2017

py/modmicropython: Add micropython.kbd_intr() function. #3011

Merged

Merge remote-tracking branch 'original/master' into uart-multiplex

d325cc2

mhoffma commented Jun 2, 2017

View reviewed changes

esp8266/uart: white space cleanup

8f8fc5f

dpgeorge reviewed Jun 2, 2017

View reviewed changes

mhoffma added 3 commits June 3, 2017 07:57

esp8266/uart: white space

ab2301b

esp8266/uart: configure default rxbuflen default to 16

fcf1aa4

esp8266/uart: white space

b2a8bcb

dpgeorge mentioned this pull request Jun 14, 2017

docs/library: Add description of "index" parameter to uos.dupterm(). #3144

Merged

dmkent mentioned this pull request Aug 16, 2017

esp8266: Allow disable of REPL on UART0 #3277

Closed

dpgeorge mentioned this pull request May 15, 2018

esp8266: Change UART(0) to attach to REPL via uos.dupterm interface. #3784

Closed

dpgeorge closed this May 21, 2018

		@@ -211,6 +244,29 @@ STATIC const mp_rom_map_elem_t pyb_uart_locals_dict_table[] = {

		STATIC MP_DEFINE_CONST_DICT(pyb_uart_locals_dict, pyb_uart_locals_dict_table);

Uh oh!

esp8266/uart: rework uart io structure so it can work with dupterm() and be used to disable stdio #2891

esp8266/uart: rework uart io structure so it can work with dupterm() and be used to disable stdio #2891

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!