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

[2.0.x] Emergency parser for multiple serial ports #10524

Merged

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Apr 25, 2018

Followup to #10516

Make sure the emergency parser can track state for multiple serial inputs.

This may need to be augmented with an additional emergent_state for each serial device, attached to whichever object is most closely associated with the serial input instance, whether it's the input ring buffer, the static class, or whatever.

Build tested for AVR, DUE, and LPC1768.

@thinkyhead thinkyhead added PR: Improvement T: HAL & APIs Topic related to the HAL and internal APIs. labels Apr 25, 2018
@thinkyhead thinkyhead requested a review from p3p April 25, 2018 13:06
@thinkyhead thinkyhead force-pushed the bf2_emergency_parser_cleanup branch 3 times, most recently from 20b2024 to dd5b6f8 Compare April 25, 2018 13:16
Copy link
Member

@p3p p3p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach, that's why I chose it for my LPC1768 PR, but not doing much AVR development I was worried about adding overhead to the UART ISR if I generalised the implementation, you know better than me that it's OK.

emergent_state is an little odd for the state variable.

@thinkyhead
Copy link
Member Author

thinkyhead commented Apr 25, 2018

I was worried about adding overhead to the UART ISR

It could all be moved to emergency_parser.h and made FORCE_INLINE, so I may do that.

emergent_state is an little odd for the state variable.

Maybe I watched too many medical dramas. I'll change it to emergency_state.

@p3p
Copy link
Member

p3p commented Apr 25, 2018

Would you be ok with adding a single character realtime command, such as 0x18 (Ctrl-X), to reset the mcu.. this isn't at all because I wouldn't have to cross the room to reset my Re-ARM to flash

@thinkyhead
Copy link
Member Author

0x18 (Ctrl-X), to reset the MCU

  • If the MCU can be reliably reset from code. There was a thread where someone couldn't get the usual methods to work at all, but maybe they did finally solve it.
  • And, if we can be sure that this character won't be spuriously sent in the event of a funky serial connection. Hinging such a strong response on a single character might be brittle, and in future if Marlin gains a binary protocol we'll need to work around it.

@p3p
Copy link
Member

p3p commented Apr 26, 2018

If the MCU can be reliably reset from code. There was a thread where someone couldn't get the usual methods to work at all, but maybe they did finally solve it.

For the ARM MCUs its just NVIC_SystemReset();, AVR I have no idea,

Hinging such a strong response on a single character might be brittle, and in future if Marlin gains a binary protocol we'll need to work around it.

0x18 will never be purposefully sent but corruption in the connection could ofc cause it, grbl have a few single char opcodes that get stripped from the serial stream before the parser, seems to work fine, but there is always the risk I guess.

@thinkyhead
Copy link
Member Author

AVR I have no idea,

An infinite loop will reset if the watchdog is on. Or, a jump to address 0x00 will do it.

@thinkyhead thinkyhead merged commit 2578996 into MarlinFirmware:bugfix-2.0.x Apr 26, 2018
@thinkyhead thinkyhead deleted the bf2_emergency_parser_cleanup branch April 26, 2018 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Improvement T: HAL & APIs Topic related to the HAL and internal APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants