32 Channel Valve Driver

I've got a working version of my relay driver board now, using PCF8575 I/O expanders and IPS6041 Smart gate drivers. The code is documented, and seems to be put together well to my un-educated eyes, but it's 300 lines in one document and at points could be confusing. I would appreciate any critique of the code, no matter how scathing. There are a few spots where I know things should be fixed and have put comments there (particularly the timing function, since I'm still trying to grasp the concepts being discussed over in Robin2's architecture thread.)

The pastebin link is: 32 Channel Relay Driver

Also, if anyone has any good suggestions on links or tutorials describing the best way to move some of this code into smaller libraries, that would be helpful to me as well since I feel a code-base this large is starting to creep up on painful.

Looks like timerData is shared between loop/setup and an interrupt service routine with no critical section. That needs to be fixed.

  server.print("{UUID:0x");

…F-macro not supported?

You have some rather large buffers plus the buffers used by the ethernet libraries. Have you checked free SRAM?

Coding,

I've updated the paste with some changes I made last night and this morning. I've wrapped the setupTimer() function with an if so that if the interrupt function is in the state of active timing ('timing' boolean is true) we don't allow any updates and respond with an error.

What does printf buy me in my replies since I have to do the psuedo-JSON formatting anyway?

To be honest, I'd never even thought of checking SRAM until your post and had no idea of how to do it. I've taken one of the functions from here and dropped it in a few of the more buffer heavy places (string parsing, etc...) Good point though that I hadn't considered.

This…

void handleAbort() {  // stops the timers and resets all ports to false when an 'abort' command is received
  timing = false;
  portABCmd = 0;
  portCDCmd = 0;
  updateOutputs();
  op = "Stopped";  // <<<<<<<<<<
}

…is guaranteed to corrupt the heap. You cannot manipulate String types in an interrupt service routine.

iyogurt: I've wrapped the setupTimer() function with an if...

As far as I can tell that does make the window smaller but does not eliminate the race condition.

What does printf buy me in my replies since I have to do the psuedo-JSON formatting anyway?

F-macro. Not printf.

Thanks for the F-macro. For anyone else viewing this later, here’s a good article explaining it. I updated the code with it.

I also eliminated the string manipulation in the ISR and have gone with using a simple byte to represent operational state. This also let me ditch the boolean of whether or not we were actively keeping time, which both did essentially the same thing. Now I can also allow for additional states down the road if need be, maybe a non-timing active state, etc…

About the race condition, the timerData array is accessed in four places (five, if you count the initialization), in the resetTimers state of parseCmd(), in sendTimers(), in setupTimer(), and in checkTimers() which is called by the ISR.

I missed adding the case to parseCmd() to prevent that, but that’s caught now, it’s also covered in setupTimer() and checkTimers(). They can each only access them one at a time now. sendTimers() should always just get the available values to send back to the user and it’s okay if it’s not 100% current (the ISR is in the middle of updating the table is fine, it’s not an exact reply while the timing is occuring anyway). Doesn’t that pretty much close the window or am I missing something?