esp2866/uart: enable/disable uart via uart_nostdio #2814
Conversation
|
What should the commit message be to meet your requirements? How do I change it without re doing the whole patch? |
|
To change the commit message, you can do a It looks like you did your changes on master. Normally you should do your changes on a branch. |
|
This is a nice feature but the implementation is a bit hacky/ad-hoc. Ultimately you'd want the buffering of UART characters to be part of the UART object, not a separate ringbuf. The input_buf should remain as-is and the UART should just be configured to either put to it or not. Similarly, whether the mp_hal_stdout_tx functions write to uart should be controlled in esp_mphal.c, not uart.c. |
|
Ok sounds like a plan we can work in that direction how should we isolate this condition. Thanks for helping me in advance. any advice on how I can get this pull request off of my master so I can resync back to the tip? |
|
I want to make sure I have the right idea here before I continue working in the right direction that make sense for the project. Damien can you please just take a quick look and make sure my partial work is as in the direction you want. Thanks in Advance. |
|
No, unfortunately, what's present in the patch now is not suitable for mainline. For last year, we're working hard to get rid of such adhoc functions like "uart_nostdio", so it would be moving backwards to now add it in. We have the means to control where terminal output goes: it's uos.dupterm() function. So, movement in the right direction would be to move all functionality required to support terminal usage completely into UART object, and then just set it by default as There will be however a regression in the user-visible behavior if done like that: with WebREPL active, a user no longer will be able to see output or provide input. @dpgeorge : Would you consider such change in behavior acceptable? My stance on this is that we'd need to do that eventually. But I did't try to go in that direction, because I'm sure the moment that goes live, we'll start to receive bunch of "reports" like "I switched term to something else and now don't see anything!!!11". So, while there're many other things to do on the port, this could wait. Alternatively, someone proposing such change should be ready to share user support load resulting from it. |
The idea of calling it "dupterm" is that it duplicates, not directs, so it should be able to handle multiple streams. Doing |
|
Yes I think so. Let me see if I get it, basically we would do
os.dupterm(machine.UART()); os.dupterm(webrepl); .... what ever you want so
there would be a list or something like that to manage the repl ports. So
we would exclude calling os.dupterm(on uart) when we want to allocate it
for something else. This is kind of nice you could envision that you could
connect the repl directly to a plain old socket if you wanted. I will get
sometime after I clean up my HW and try to make some progress here.
…On Sun, Feb 12, 2017 at 8:42 PM, Damien George ***@***.***> wrote:
Would you consider such change in behavior acceptable?
The idea of calling it "dupterm" is that it *duplicates*, not directs, so
it should be able to handle multiple streams. Doing
os.dupterm(machine.UART()) followed by os.dupterm(webrepl), should put
the REPL on both streams. Then it needs a way to un-duplicate, ie remove a
stream from the list.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2814 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQEAU-lKLaOCPSzAtEDKfsL_3u1oyfy9ks5rb7TqgaJpZM4LwYEw>
.
|
Yes, that's how WiPy implemented it, and would be nice to have decided what to do about that. WiPy's implementation is too bloaty - it requires an underlying list object to store all the streams, non-trivial error handling in case of stream errors, and as you noticed, another function to "un-duplicate". Well, exactly because of all these issues, we didn't implement dupterm() as a list in esp8266. And to remind, esp8266 was supposed to be the latest-greatest port, not just another port "doing it differently". Consequently, I think it makes sense to "officially approve" the esp8266 design and just have a single slot for terminal stream, which can be set by uos.dupterm(), which call returns the previous stream used. Nice and simple. Someone who needs to send terminal to 2+ destinations, can develop (then debug, then support) a multiplexer class. Of course, if you really think that we should do bloaty way, we can. |
|
Let me take a look at this it seems like you really simplified the entire
design. So dupterm takes a stream object and returns the previous stream
object and the esp8266 init code will connect uart0 to dupterm at startup.
There shall be 1 and only one terminal connected at any one time.
#if MICROPY_PY_OS_DUPTERM
mp_obj_t term_obj;
mp_obj_t dupterm_arr_obj;
#endif
STATIC mp_obj_t mp_uos_dupterm(mp_uint_t n_args, const mp_obj_t *args) {
mp_obj_t prev = MP_STATE_PORT(term_obj); // init to mp_const_none ?
if (prev == MP_OBJ_NULL) old = mp_const_none;
if (n_args != 0) {
if (args[0] == mp_const_none) {
MP_STATE_PORT(term_obj) = MP_OBJ_NULL;
} else {
MP_STATE_PORT(term_obj) = args[0];
if (MP_STATE_PORT(dupterm_arr_obj) == MP_OBJ_NULL) {
MP_STATE_PORT(dupterm_arr_obj) = mp_obj_new_bytearray(1,
"");
}
}
}
return prev;
}
is this what you are thinking?
void mp_hal_init(void) {
//ets_wdt_disable(); // it's a pain while developing
mp_hal_rtc_init();
// uart_init(UART_BIT_RATE_115200, UART_BIT_RATE_115200);
mp_uos_dupterm( pyb_uart_type.make_new(...) )
}
void mp_hal_stdout_tx_char(char c) {
// uart_tx_one_char(UART0, c);
mp_uos_dupterm_tx_strn(&c, 1);
}
...
…On Mon, Feb 13, 2017 at 3:06 AM, Paul Sokolovsky ***@***.***> wrote:
The idea of calling it "dupterm" is that it duplicates, not directs, so it
should be able to handle multiple streams. Doing os.dupterm(machine.UART())
followed by os.dupterm(webrepl), should put the REPL on both streams. Then
it needs a way to un-duplicate, ie remove a stream from the list.
Yes, that's how WiPy implemented it, and would be nice to have decided
what to do about that. WiPy's implementation is too bloaty - it requires an
underlying list object to store all the stream, non-trivial error handling
in case of stream errors, and as you noticed, another function to
"un-duplicate". Well, exactly because of all these issues, we didn't
implement dupterm() as a list in esp8266. And to remind, esp8266 was
supposed to be the latest-greatest port, not just another port "doing it
differently". Consequently, I think it makes sense to "officially approve"
of esp8266 and just have a single slot of terminal stream, which can be set
by uos.dupterm(), which returns the previous stream used. Nice and simple.
Someone who needs send terminal to 2+ destinations, can develop (then
debug, then support) a multiplexer class.
Of course, if you *really think* that we should do bloaty way, we can.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2814 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQEAU5v8Dipk4Q5KNtaQ6dPc15My2DLpks5rcA8UgaJpZM4LwYEw>
.
|
No, cc3200 port just has a single slot for the dupterm object which is overwritten by each call to dupterm() (and it actually caches the read/write methods to make it more efficient; also has a special case for UART to make that efficient as well). It always sends/receives on the telnet so that's why it's considered duplication. So there will be ports (eg cc3200, stmhal) that have a default and always-on REPL, and then os.dupterm() is used to get an additional REPL on some interface.
Calling dupterm() without args already returns the current stream, do you propose changes to that behaviour?
I know that with stmhal I often find use for having 2 destination for the REPL, the USB and a UART, and I wouldn't want to change that. A simple way of supporting multiple channels is to have a fixed number of possible duplications (eg 2) and then have an optional second arg to dupterm() which specifies the slot to use. Then ports that don't have the room (code and/or RAM) can just support 1 slot and the API is unchanged for them. |
|
Paul I think it's wrong calling it dupterm if we change it to a single slot
its really no longer dupterm. Are you thinking that we would have a slot
that we can just assign any stream interface object to?
…On Tue, Feb 14, 2017 at 2:32 AM Damien George ***@***.***> wrote:
Yes, that's how WiPy implemented it
No, cc3200 port just has a single slot for the dupterm object which is
overwritten by each call to dupterm() (and it actually caches the
read/write methods to make it more efficient; also has a special case for
UART to make that efficient as well). It always sends/receives on the
telnet so that's why it's considered duplication.
So there will be ports (eg cc3200, stmhal) that have a default and
always-on REPL, and then os.dupterm() is used to get an additional REPL on
some interface.
Consequently, I think it makes sense to "officially approve" the esp8266
design and just have a single slot for terminal stream, which can be set by
uos.dupterm(), which call returns the previous stream used.
Calling dupterm() without args already returns the current stream, do you
propose changes to that behaviour?
Someone who needs to send terminal to 2+ destinations, can develop (then
debug, then support) a multiplexer class. Of course, if you really think
that we should do bloaty way, we can.
I know that with stmhal I often find use for having 2 destination for the
REPL, the USB and a UART, and I wouldn't want to change that. A simple way
of supporting multiple channels is to have a fixed number of possible
duplications (eg 2) and then have an optional second arg to dupterm() which
specifies the slot to use. Then ports that don't have the room (code and/or
RAM) can just support 1 slot and the API is unchanged for them.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2814 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQEAU2Ymmz5gwDE1bYFBlI-zJXRXhVWgks5rcVhwgaJpZM4LwYEw>
.
|
Maybe I mixed it up with other piece of code in cc3200 port vividly distinct by using lists, I'm writing by memory.
Even if I do, returning an old value when called with an argument, is not change to behavior, but addition to it ;-).
So, that already adds "more than one" complexity, but doesn't give enough flexibility. How would WebREPL work portably across such systems for example? Or do we just assume that slot 0 is the default for all tools to use (as that's the only one guaranteed to be), and all slots above zero are "user's"? In such formulation it may be not that bad. (So, for example, stmhal would init default REPL at dupterm no.1 and and leave no.0 empty). |
If you're actually into developing patch per requirements above, that would be the last thing to care ;-). Even if name changes, it's a trivial thing to fix. I personally don't think that the name should change, it gives good enough hint what a function do regardless of details how it works - for example, it takes a UART object, and duplicates it to serve as a terminal object. Try to argue that word "duplicates" used above is completely wrong ;-). |
Yes, if you're interested, you can start with assumptions above, while the details of external API is being settled. The biggest part of that patch would be not those details, but making sure that UART object can be used with dupterm() - without regressing how REPL works currently, and also without regressing native UART functionality. |
|
I'm trying to draw a nice picture around the flow we currently have so I can make sense out of it and its seems to have a lot of stuff going on. Lots of dependencies spread out over a few files.
I started to pull pieces in from cc3200 as suggested and before I try to make this work and break off to make a new pull request I wanted to get a little more constructive input. The nice thing about looking at this code is I can see now how Damien wanted the uart buffer arranged. There really is nothing like jumping into a project at the core and chewing off more than one can chew in order to understand the architecture. I think I might need to employ the use of gdb or at least a hardwired led to get through the debug of this code. I really like the event flow of esp better than what is done on cc3200 it seems like a better design when it comes to dealing with the event loop. I need to unwind this a bit more, the loop in cc3200 uses a for(;;) loop and a time delay to get chars for ever. I like the notify method being used in esp it feels much better. the patch has both not working can you guys give me a good suggestion on how to keep this code:
|
|
@mhoffma nice diagram! But your attached diff is really difficult to comment on. Please follow @pfalcon's latest suggestion: make it so that you can do uos.dupterm(uart), to duplicate the terminal on the uart a second time. Yes that would make it output twice on the uart but it's a step in the right direction. |
Actually, this already works. So that's good, it means machine.UART can already be used as an entry in a dupterm slot. |
Fix micropython#2814 Corrected UART output.
This is a feature developed in the forum by (brad) which allows us to enable and disable the uart on ESP8266 via the function uart_nostdio. This feature enables us to re-purpose the uart after other communication methods have been established i.e. wifi is up.
here is a pointer to the thread.
https://forum.micropython.org/viewtopic.php?f=16&t=1809&start=30