-
Notifications
You must be signed in to change notification settings - Fork 128
Added SERIALCOMMAND_ECHO macro to enable serial echo #11
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments on your code.
In general, I like the idea and will merge if you fix the small issues.
SerialCommand.cpp
Outdated
@@ -124,6 +124,11 @@ void SerialCommand::readSerial() { | |||
#endif | |||
} | |||
} | |||
else if (inChar == 127 || inChar == 8) { | |||
bufPos--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a possible buffer-overflow when deleting more characters than there are in the buffer (i.e. bufPos
rolls over from 0 to 255).
SerialCommand.cpp
Outdated
@@ -124,6 +124,11 @@ void SerialCommand::readSerial() { | |||
#endif | |||
} | |||
} | |||
else if (inChar == 127 || inChar == 8) { | |||
bufPos--; | |||
buffer[bufPos] = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use '\0'
here instead of 0
to make clear we're writing a character.
SerialCommand.cpp
Outdated
else if (inChar == 127 || inChar == 8) { | ||
bufPos--; | ||
buffer[bufPos] = 0; | ||
Serial.print(" \b"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the space before the \b
for?
Also: wrap in #if defined SERIALCOMMAND_DEBUG || defined SERIALCOMMAND_ECHO
!
SerialCommand.cpp
Outdated
@@ -1,23 +1,23 @@ | |||
/** | |||
* SerialCommand - A Wiring/Arduino library to tokenize and parse commands | |||
* received over a serial port. | |||
* | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove whitespace-only changes from your PR.
Will take a look. Thank you for your comments — I’m sure I’ll wanna fix
that for myself too.
I don’t remember what the `[space]\b` is for right now. I’ll have to look
at it.
How do I remove whitespace-only changes? I’ve never figured that out
…On Fri, May 25, 2018 at 10:29 AM kroimon ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Please see my comments on your code.
In general, I like the idea and will merge if you fix the small issues.
------------------------------
In SerialCommand.cpp
<#11 (comment)>
:
> @@ -124,6 +124,11 @@ void SerialCommand::readSerial() {
#endif
}
}
+ else if (inChar == 127 || inChar == 8) {
+ bufPos--;
This creates a possible buffer-overflow when deleting more characters than
there are in the buffer (i.e. bufPos rolls over from 0 to 255).
------------------------------
In SerialCommand.cpp
<#11 (comment)>
:
> @@ -124,6 +124,11 @@ void SerialCommand::readSerial() {
#endif
}
}
+ else if (inChar == 127 || inChar == 8) {
+ bufPos--;
+ buffer[bufPos] = 0;
Please use '\0' here instead of 0 to make clear we're writing a character.
------------------------------
In SerialCommand.cpp
<#11 (comment)>
:
> @@ -124,6 +124,11 @@ void SerialCommand::readSerial() {
#endif
}
}
+ else if (inChar == 127 || inChar == 8) {
+ bufPos--;
+ buffer[bufPos] = 0;
+ Serial.print(" \b");
What's the space before the \b for?
Also: wrap in #if defined SERIALCOMMAND_DEBUG || defined
SERIALCOMMAND_ECHO!
------------------------------
In SerialCommand.cpp
<#11 (comment)>
:
> @@ -1,23 +1,23 @@
/**
* SerialCommand - A Wiring/Arduino library to tokenize and parse commands
* received over a serial port.
- *
+ *
Please remove whitespace-only changes from your PR.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#11 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARYddZ_LESz9weFT7Kd6V-NQyUfHXM-ks5t2BVhgaJpZM4KLPpS>
.
|
I didn't want to enable full on debug mode, but I want to be able to see my commands as I type them. So I added an "echo mode."