High Reliability Serial Firmware - Library and Template

Status
Not open for further replies.
N

neltnerb

Well-known member
For those of us who write a lot of firmware for instruments that is controlled via USBSerial, I've found very few good patterns or libraries for doing this in a way that does things like serial parsing, argument type conversion, and registering of functions. The result is often messy and buggy code that's really hard to maintain or even follow the flow of, so I wanted to write a library to do these common tasks. Particularly when you need to do parsing, the lack of regex means really messy nested if loops that I found to be very error prone in writing them properly.

As such, I've created this:

https://github.com/neltnerb/Teensy-Reactor-Controller

which has an example for controlling a valve, includes an example of using an interrupt, and also demonstrates a best practice for using elapsedMillis to do non-blocking work.

The checkSerial() function automatically looks for new characters on the Serial device and puts them into a buffer to provide expandable buffer length. It properly handles backspaces for interactive control, and accepts both carriage returns and newlines as a termination character so that the mess that is often LabView code is more likely to work on the first try.

checkSerial() prints errors to the serial port for things like attempting to access an argument that doesn't exist, invalid conversion (say from "0.0" to a type int), or out of range errors, and provides a method for retrieving the number of arguments received to do manual checking for exactly the desired number of arguments (or allows for variable argument numbers). Upon an error it returns immediately from the handler and prints a message rather than potentially doing something unintended, which is my preference for things that control toxic chemical flow paths.

It also provides the special serial command "ListCommands" which simply provides a list of all of the registered commands and their descriptions.

I tested command parsing with a Teensy 3.1, and it has no issues I've found yet by trying to do all of the myriad horrible things often done on a Serial interface to the poor firmware. It would be lovely to have other people testing it too if anyone thinks this would be helpful to them. It also compiled for Arduino ARM targets, but the presence of vectors made it not compile for regular AVR. I thought about trying to replace that with a linked list or fixed length char* but since I'm personally just using the Teensy at this point I found this easier. There seem to be AVR implementations of the vector library, but I didn't want to deal with that myself since I'm not using the chips.

I didn't set it up yet so that it's possible to select different serial interfaces for the input character stream. I think something like checkSerial(Serial device) might work, but I haven't thought about the implications fully so for now it just assumes you're using the main USB Serial port.

Happy to accept revisions, bug fixes, or hate mail depending ;-) I'm definitely not a professional programmer, I just ended up having to write piles of firmware for devices I've built for custom systems and run into the same set of problems every time. I think this kind of thing will make it a lot better, even moreso if others can add to it for a common well-tested library.
 
I'm on mobile right now but I will try this once I'm home. Sounds great.
 
Hi,

I had a look at the SerialEval Library code. Unfortunately I can't test the code on a Teensy because all my Teensies are at my desk at university :rolleyes:. I will do some tests on monday. Anyway, here are my thoughts so far:

command(name) looks like a function call, but you define
Code: 
#define command(name)  static void name(std::vector<String> argv)
so it really is a declaration (or definition) of a function, right?

I'm not exactly a C++ wizard but tried to understand the code anyway. I think I understand you create a vector of "Command"s, which is a struct that contains:
- a name string to test the serial input against
- a description string for the "ListCmmands" output
- a pointer to the handler function

And here...
Code: 
for (unsigned int i=0; i<CommandVector.size(); i++) {
      if (CommandVector[i].name == parsed_argv[0]) {
        CommandVector[i].handler(parsed_argv);  //<--here
        CommandHandled = true;
      }
}
the correct handler function is called.
... right? :confused:

I really like this library and I'm ok with referencing/dereferencing variables, like it's done in the _parse_x functions. But if you want to reach a bigger audience in the arduino word, exposing pointer stuff to the user is something you want to avoid. I't less confusing for beginners to use the function return variable for the actual return instead of the "returns 0 if executed with no errors"-paradigm. Using the function return means you can do things like
Code: 
int myInteger = intArg(1);
which is self explaining, provided the function names are not too cryptic.

I'd also recommend to put this into a class to arduinofy the programming, e.g. with a ::begin function, even if that's not needed. You could use a constructor argument to specify the serial interface or, if there shall only be a single set of commands per device, maybe a ::attachInterface function to define where the input stream may come from. Using instances of a class each interface could have its own set of commands (e.g. one interface to talk to LabView and another interface for status reports to a terminal...).

Have a look at this:
https://www.arduino.cc/en/Reference/APIStyleGuide

Thanks again for sharing your work with the public, this already is a very useful library despite its early stage!

Ben
 
Thanks so much for the close look and feedback!

Right, command(name) creates a static function called name with argv as a variable. The basic premise for defining it with a macro like this is that it allows me to hide the argv "under the hood" so that the person using the library isn't needing to think about how the serial data ends up as variables they can use. I would have preferred some construction where variables are parsed in the checkSerial() function itself and then passed as inputs to the function, but that was too tricky for me to figure out. Passing them all as a single internally hidden string was the best we came up with as a compromise.

As for how it works, yes, you're right -- Command is a struct that contains the pointer to the handler function, which lets you call it in that way. I was trying to copy the style of interrupt handlers where the user defines a static void function and then references it with the interrupt handler registry. The big difference is that for this application there are inputs that should be sent to the handler whereas in an interrupt it can't accept any formal arguments because the event is triggered by hardware. You can see the overall design as attempting to parallel interrupt handlers but triggered by seeing specific serial messages with content available about what that message was.

Yes, that snippet of the code walks through the CommandVector one element at a time, checks for a match of name against the first space delimited String in the serial data, and if it matches it runs the handler function.

The parse functions didn't return that way because if they error they don't just return non-zero, they *break out of the command handler* as if the command sent was invalid. It seems from asking my friend who helped me figure out that part that you can't break across called functions, so it was necessary to do it via a macro. In case you had also never run much into macros, I understand them to just be defines that replace a thing that looks like a function call with an in-line block of code.

After some searching and help from said friend, it seems to be possible to make a macro appear to return a value via the gcc extension for expressions:
https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
so I just tried that, it seems to work for int at least, and the changes are now on the github. Not sure if that would work on AVR though...

(Updated: https://github.com/neltnerb/Teensy-Reactor-Controller)

In my opinion, the goal is to put a heavier burden on the end user to send proper commands to make anything happen, and that reliable firmware needs to take serious action if it receives the wrong type of variable in the serial command -- i.e. if someone misreads the API and sends ON instead of 1, and you tried to convert it using the standard String.toInt() function, it returns 0 for an invalid conversion. So, if I'm not checking to be sure that the user of the device is making a valid conversion in that case the ON would return 0 because it was invalid, and then turn the valve off instead of on. This is very, very bad behavior, so in this setup the checkSerial() and intArg() functions print a descriptive error back to the Serial interface and then stop the command itself to prevent incorrect assumptions about the variable validity from passing without notice.

Definitely opening to hearing other ways of approaching the issue of users of the device (not the library, but an end user) not reading the manual (sounds like you've been there) in ways that you (as the firmware programmer) didn't think anyone could be so careless as to do :D I know most of the craziness, wrong numbers of arguments, wrong termination characters, wrong variable types, putting random quote marks, ... so my main goal was to err on the defensive and make it check for everything for every variable and to tell the end user they're doing it wrong.
 
Last edited:
Oh, FWIW, the only reason I put the details of how command(name) actually creates a function called name in the library usage is because I was worried someone might create a command with the name and possibly create a function called name elsewhere. I'd prefer to have just not mentioned the under the hood stuff at all in the documentation, but I can imagine that happening pretty easily and people being very confused when they get an error for their function being defined multiple times.
 
What would perhaps be really, really, really cool is if I were able to hide even the checkSerial() command from the main loop and do interrupt driven serial parsing and command evaluation... I'm hesitant to use interrupts on Serial Receive for parsing in case a command function takes a long time to return and blocks additional Serial reception from buffering.

Maybe this is a question for Paul as to the best way to achieve that. It would be really incredible to make the analogy to hardware interrupts complete by making it so that a Serial Command being received is automatically handled without user intervention, but the whole blocking aspect of real interrupts means I'd need a much cleverer way to do it (I think).
 
neltnerb said:
It would be really incredible to make the analogy to hardware interrupts complete by making it so that a Serial Command being received is automatically handled without user [application] intervention
Click to expand...
Hmm, exceeding the common need?
 
sorry.. Did you mean that the ISR would process some commands rather than get in, get out and let the non-ISR application handle the arriving data?
 
I think that you're saying what I was saying, yeah.

It would be neat if the Serial handler and commands executed without user intervention by running checkSerial(), but I think doing that would require monitoring for a serial interrupt and then executing the handler while inside the serial interrupt -- that would be bad because it would block all other interrupts while executing a potentially CPU intensive function.

It would be a nice compromise if there were some clever way to make checkSerial() run at periodic intervals from inside the main loop without it being explicitly called, but telling the user of the library to do so doesn't seem particularly burdensome.
 
Thanks for the extensive explanation, I now comprehend the design decisions you made. But I'm still not entirely comfortable with them, although I don't have anything better to offer. The use of compound statements to have return values looks great.
I think it is necessary to make a fundamental design decision:
You either leave the internal structure of the library as it is. This means you sacrifice the conformity to the standard arduino library style and expect the end user to understand the concept of pointers.
Or you respin the internal workings or provide a wrapper class to arduinofy the library. I must confess I have no clue if and how that would work.:confused:

I wouldn't bother trying to hide checkSerial from the user, the user can use IntervalTimer or elapsedMillis to define the polling frequency himself.

Ben
 
neltnerb said:
It would be a nice compromise if there were some clever way to make checkSerial() run at periodic intervals from inside the main loop without it being explicitly called, but telling the user of the library to do so doesn't seem particularly burdensome.
Click to expand...
Arduino and Teensyduino don't natively have a threaded or multitasking scheduler. Nor even a finite state machine with run-to-completion as a simple scheduler.
One can use FreeRTOS or Chibios. With such, one thread can block for interrupts, or poll for incoming things and the other threads/tasks can do whatever.

Lacking such a scheduler, the ISR tells the non-ISR code to process using a private agreement such as a flag (volatile attribute) or a message queue.
Another scheme that's the same is for the application to tell the ISR the address of a user-function (call-back) to process incoming data. But this function must behave like an ISR. It does get the user code out of the ISR - a good thing, and commonly done so the ISR can be generic.

The best thing, as above, is to NOT poll but use event driven programming. But the AVR class microprocessors are often too short on RAM for such.

All this has just followed the simple Arduino concepts over time.

Newer enviornments take a day-1 different approach AND keep it simple for Arduino-ites. A Chibios based example is the Python oriented Viper software on one of several boards including the low cost Photon WiFi board. Its RTOS is there but the user code isn't really aware of threading within the overlapped I/O in the drivers. (But user programs can create threads too.)

We hope that these notions could come to a new Teensy.
 
Thanks Steve for the insight! It does make sense when you think about the evolution from assembly PIC16 programming through now, I'm so used to counting estimated instructions after compilation while I'm going that the idea of releasing that fine control to a scheduler is definitely a bit of a radical departure. It would be really nice though to have something that allowed for scheduling tasks without being a full OS! Something that looked closer to the underlying C code but which had a scheduler...
 
Mostly, on small embedded, I use one or more finite state machines (FSM) rather than a preemptive scheduler.
Single stack = simple, in terms of mutual exclusion and avoiding deadlocks.
Just 50 lines of code, or so.

Not a lot of people understand the merits of concurrent FSMs versus a cooperative scheduler vs. a preemptive scheduler. It's an important foundation for serious work.
 
stevech said:
Mostly, on small embedded, I use one or more finite state machines (FSM) rather than a preemptive scheduler.
Single stack = simple, in terms of mutual exclusion and avoiding deadlocks.
Just 50 lines of code, or so.

Not a lot of people understand the merits of concurrent FSMs versus a cooperative scheduler vs. a preemptive scheduler. It's an important foundation for serious work.
Click to expand...

Can't argue with that, learning finite state machines was probably the single biggest factor in my firmware starting to be both reliable and readable. Maybe it would be good to make a nice template for implementing them; I feel like once you see how to convert a state machine diagram into a switch with an enum state (or even just how to think about things as states and transitions), life is much better for all involved.
 
Status
Not open for further replies.
Back
Top