Paciente8159 / uCNC

µCNC - Universal CNC firmware for microcontrollers
https://github.com/Paciente8159/uCNC/wiki
GNU General Public License v3.0
265 stars 60 forks source link

Only skip DEBUG_STREAM when it is not default_stream #664

Closed lonelycorn closed 5 months ago

lonelycorn commented 5 months ago

There is a bug in serial_available() that will make the system ignore any incoming bytes from default_stream. With UART as default_stream, the system will stop talking to a sender software on a host machine.

To repro:

  1. enable DEBUG_STREAM (mapped to default_stream)
  2. register multiple serial streams
  3. build and flash
  4. connect system to a sender via UART, and verify the two can communicate
  5. send data via any stream that is not default_stream (i.e. not UART)
  6. sender can no longer communicate with the system

This diff fixes the bug by skipping DEBUG_STREAM only when it is not default_stream.

TBH I am not sure why we want to skip DEBUG_STREAM in serial_available(). serial_available() checks number of bytes in the RX buffer. In almost all scenarios DEBUG_STREAM is used to send debug messages out; RX buffer isn't that useful. We might as well just remove the check

Paciente8159 commented 5 months ago

That is intentional. Port streams are registered in a fixed order (if available). Default stream points to the first registered stream

  1. UART
  2. UART2
  3. USB
  4. WIFI
  5. BLUETOOTH

So lets say your board has UART and USB available (making UART your default stream) and you ENABLE_DEBUG_STREAM but you want to use USB as your debug stream. You can modify that by also setting debug stream like this.

// note that this take the pointer to the stream

define DEBUG_STREAM (&usb_serial_stream)

while the serial_available is checking the available streams for available characters it can skip the usb stream (that is not the default stream).

Note that other types of streams can be used as a debug stream. Like for example a custom stream to an SD card and create some sort of data logger as well.

lonelycorn commented 5 months ago

the point I was trying to make is that the "default setting" (i.e. mapping DEBUG_STREAM to default_stream) just works out-of-box and I can see both regular responses and debug messages from a sender, but stops working when another stream is added. that is confusing

My suggestions:

Paciente8159 commented 5 months ago

the point I was trying to make is that the "default setting" (i.e. mapping DEBUG_STREAM to default_stream) just works out-of-box and I can see both regular responses and debug messages from a sender, but stops working when another stream is added. that is confusing

When you only have on single stream (default_stream) stream multiplexing does not happen and you can use default stream as usual (that is the standard protocol plus the debug prints). When you add a second stream this picture changes. As soon as the stream multiplexer starts switching streams the stream assigned to act as debug stream stops accepting commands (works only as an output print for debug messages and broadcast messages like the status message). That is why you get the felling that is stops working. If you add a second stream the debug stream stops accepting commands but this is made by design to remove interference from that stream.

But I understand what you are saying. I could still allow the debug stream accept commands like any regular stream leaving the open possibility to send some sort of command back via this stream.

Removing this port exclusively to debug print could be exerted by also enabling the DETACH_FROM_MAIN_PROTOCOL option (this will also make it stop printing broadcast messages).

lonelycorn commented 5 months ago

If you add a second stream the debug stream stops accepting commands but this is made by design to remove interference from that stream. a second stream may not necessarily be a physical stream. for instance, <uCNC>/src/modules/system_menu.h mentions a serial stream is needed to send commands (e.g. jogging control): https://github.com/Paciente8159/uCNC/blob/377703da17309049bfd0f5b68fff483405ccbcad/uCNC/src/modules/system_menu.h#L191-L192

anyhow I don't think there's a right vs wrong discussion. all I wished was to remove confusion. if it is by design then I'd suggest to "enforce that DEBUG_STREAM be mapped to a different stream than default_stream when there are multiple streams"

Paciente8159 commented 5 months ago

That is an excellent point. Agree.