You need to define what you mean when you say better. Faster, less memory, more resilient, easier to maintain, easier to extend, something else ?
How about this ?
Is it better than either of the them ?
const int ledPin = 13;
const int ledPin2 = 12;
const int ledPin3 = 11;
char incomingData = 0;
void setup()
{
// put your setup code here, to run once:
Serial.begin(115200);
pinMode(ledPin, OUTPUT);
Serial.setTimeout(50);
digitalWrite(ledPin, LOW);
}
void loop()
{
if (Serial.available() > 0)
{
incomingData = Serial.read();
if (incomingData == 'A')
{
digitalWrite(ledPin, Serial.parseInt());
}
else if (incomingData == 'B')
{
digitalWrite(ledPin2, Serial.parseInt());
}
else if (incomingData == 'C')
{
digitalWrite(ledPin3, Serial.parseInt());
}
}
}
What happens if you send only a singe character ?
What have you got Line ending set to in the Serial monitor and should the code take that into account ?
Incidentally, none of teh sketches, including mine, set the pinMode() of the ledPins properly
I would say that using Serial.parseInt() is risky businesss because it can timeout and return 0 and you don't know if you actually read 0 or not.
I would do something fully asynchronous as a state machine, like this
enum ReadState { WAIT_FOR_LINE, WAIT_FOR_STATE };
const char minLine = 'A';
const char maxLine = 'C';
bool readCommand(byte &line, byte &state) {
static ReadState currentState = WAIT_FOR_LINE;
static byte tmpLine = 0;
int c = Serial.read();
if (c == -1) return false;
switch (currentState) {
case WAIT_FOR_LINE:
if (c >= minLine && c <= maxLine) {
tmpLine = c - minLine; // extract the line number
currentState = WAIT_FOR_STATE;
}
break;
case WAIT_FOR_STATE:
if (c == '0' || c == '1') { // valid state
state = c - '0';
line = tmpLine;
currentState = WAIT_FOR_LINE;
return true;
}
else if (c >= minLine && c <= maxLine) { // handle cases like A B 1 which should still find B 1
tmpLine = c - minLine;
}
else if (! isspace(c)) { // skip spaces
currentState = WAIT_FOR_LINE;
}
break;
}
return false;
}
void setup() {
Serial.begin(115200);
Serial.println("Ready");
}
void loop() {
byte line, state;
if (readCommand(line, state)) {
Serial.print("led on line "); Serial.print(line);
Serial.print(" set to "); Serial.println(state ? "High" : "Low");
}
}
The function isspace() returns true for any whitespace character, which includes space ' ', tab '\t', carriage return '\r', and line feed '\n'. So both CR and LF are treated as whitespace by isspace() which means the code should work if you type A B or C and return in the Serial monitor (whatever its config) and then type 0 or 1 and return. if should work also if you type in one go A 0 B 1 C 1 or A0B1C1 etc.. basically spaces are ignored.
Personally, I prefer @J-M-L ‘s approach, it’s a lot more scalable, and uses better strategies for expansion and input exceptions.
Ideally, you might develop this to offer an input buffer that you can parse and ‘escape’ from, as well as extra parameters - flash, dual, alternating etc.
As long as you use state = Serial.parseInt(); you introduce a timeout and some weakness. if the state is not received within 1 second after the pin identifier, parseInt() times out and returns 0. At that point you can't tell if that 0 was because the end user actually sent the command LOW or just did not send anything...
So that's a weakness. You might be fine with it, just know it's there.
Whether it is "better" or not depends on what @hk_jh means by that and they haven't said
It is often the case when dealing with user text input that the code to make sure that it is formatted correctly and will not cause problems is larger and more complicated than the code to deal with properly formed input. When prompted to enter a number the code needs to deal with anything entered, including non numeric input and any range of numbers including negative and floating point numbers
We have, of course, wandered away from the original question which was which of two imperfect sketches was better, but that is no bad thing
Or, for SAG, define your state within your command. Use ‘A’ to set the first output to a 1, use ‘a’ to set the same output to 0. No parsing, simple, straightforward.
(Sorry, had a requirements discussion some time ago)
With respect to the word “better”, how many of the proposed examples are tested to handle input that is “out of spec” as defined in the post #1 ?
Some out of spec inputs { A2 a0 a1 A10 A15 AA AB AB1 BD1 A0D1 }, and note there is a space between A and 0 etc. It is not clear from the specs if this space is optional or not.
the trailing 2 will be part of the value for the next letter command
'return' is neither a digit nor printaable, therefore ignored
i believe any value other than zero is treated as HIGH by digitalWrite()
an 'a' or 'E' would be translated to a idx (= c - 'A') > Nled and ignored
shouldn't the code ignore A, recognize B 1 ignore A and recognize C 2 as per OP's initial requirements (and wonder what to do with 2 - my take is that is should be ignored and thus only B 1 should occur).
not in the code i posted. (default value of the argument is zero)
did the OP sepcify requirements simply describe what is being done.
preceding the cmd with the value simplifies the code by avoiding the need to recognize the command and then wait for the argument which may be multiple characters. multiple commands can be concatenated on the same line or straddle lines with the code i posted.
for handling a more conventional approach: cmd argument -- i believe it's better to capture an entire line using Serial.readBytesUntil() and then extract a cmd string and argument using scanf()