Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only skip DEBUG_STREAM when it is not default_stream #664

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

lonelycorn
Copy link
Contributor

@lonelycorn lonelycorn commented Mar 6, 2024

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

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 not be able to communicate with 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, 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.
@Paciente8159
Copy link
Owner

Paciente8159 commented Mar 6, 2024

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
Copy link
Contributor Author

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:

  • enforce that DEBUG_STREAM be mapped to a separate stream than default_stream; or
  • do not skip DEBUG_STREAM in serial_available() because 1) debug streams are almost exclusively for writing data out; 2) most of the debug commands are sent via UART, i.e. default_stream

@Paciente8159
Copy link
Owner

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
Copy link
Contributor Author

lonelycorn commented Mar 15, 2024

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):

// this needs to be implemented using a serial stream
uint8_t system_menu_send_cmd(const char *__s);

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
Copy link
Owner

That is an excellent point. Agree.

@Paciente8159 Paciente8159 merged commit 970d920 into Paciente8159:master Mar 15, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants