Thread by @EshlemanMatthew

This is great. The only time I get to see code like this is typically under NDA while under development or while under a court protective order.

First review note: traditional older style C code, but looks professional. Some encoding issues in the comments...

https://twitter.com/mbeddedartistry/status/1245476560758427650

HMI code is c++ though. Appears to be custom framework, not sure yet.
French comments I believe.
Visual Studio Code is handling the code fairly well.
Qt Creator IDE use will need effort.
References to Keil and Borland.

<sigh> My old enemy: busy loops to enable delays, waits, etc.

Tasking compiler reference too. Haven't seen that reference in a decade (or two?)

different main loop "schedulers" based on whether a button was pressed during power on. Interesting.

task based "schedulers". Fully custom, traditional bare metal firmware, no RTOS that I see .

Maybe find time to look more tomorrow. First impression: this is professional "super loop" style firmware. I don't recommend this style, but it is fairly typical.

I recommend:
covemountainsoftware.com/2016/08/31/pre…

The LX010106 folder:

ansic: 55330 (56.71%)
cpp: 41641 (42.68%)
python: 520 (0.53%)
asm: 72 (0.07%)
Total Physical Source Lines of Code (SLOC) = 97,563

another view:

basically a 2:3 ratio on comments to code.

ongoing observations on medtronic ventilator code.
Just ran cppcheck on the code base.
Minimal concerns, but a few critical

A NAND Flash function to read the NAND device ID has a definite array read overflow, thanks to a magic number if a for loop that is off by one.

Another function has a if statement that is accessing an array with an index, but checks the index afterwards, resulting in possible out of bounds array read.
Kinda like this:

if ((Table[index].id == NO) || (index >= MAX ))

A fair number of "member variable is not initialized in the constructor" warnings in the HMI C++ code. Sadly typical oversight.

with cppcheck, this is a reasonably clean code base. Sort of surprised they didn't find the array out-of-bound accesses, but only a few found. Not bad!

Nice. This is another sign of a generally clean and professional code base: most functions have low complexity.

Lots of complexity in USB driver code.

But some of those complex functions are where we might expect: code calculating "flow leak", "exhalation", "inspiration", "O2", etc.

lots of static variables inside functions tracking past values. converting to TDD/unit testing would be tricky.
Maybe @jwgrenning is up to the challenge! :-)

cppcheck also found :

val = (2932 * (SWORD32)AD_Digit.Volt24) / 10000;
val = val; //cppcheck calls this out.

I see several places with that pattern, but typically in the context of #ifdefs and avoiding unused var warn. Not sure why they did this in this case? timing? mistake?

One malloc within a func enabling C++ new. Only one 'new' allocation in the HMI code but limited to BORLAND compiler only.

If I'm reading correctly, everything is statically allocated. This is normal, especially in FW that likely went through a certification/reliability process.

@threadreaderapp unroll