Refactoring bad design?

Question

Could someone help me how to refactor old very complex code ?

Original used just setup and everything else was in single ISR. But as it was bigger and bigger, code is quite slow and quite random sometimes, even PC emulator did not helpped much (probably some long operations like printing are sometimes interrupted by next interrupt depending on analog values, buttons, etc.).

Is there any guide how to maintain such a project or is there only way out start again from scratch ?

As there are few important regulations also event queue would be needed, but not sure if there is any pattern or a template to start from in case of restart. A-Star 32U4 Micro has <32kB flash and code is hard to fit in yet (there are also big fonts).

Example of font unpack code, but hope this part is quite ok:

void Adafruit_GFX::drawChar(int16_t x, int16_t y, unsigned char c,
  uint16_t color, uint16_t bg, uint8_t size) {
...
    uint8_t blockSize = 0;
    if(packed) blockSize = pgm_read_byte(&bitmap[bo++]); // for first char pick 1st bit array only
    BitStreamsReader bitStream(&bitmap[bo++], packed?blockSize:0); // ptr, blockSize(if packed)
    BitXorBuffer rowXorBuf(w); // row XOR buffer (each row is XORed with)
    bool chk;
    for(yy=0; yy<h; yy++) {
        rowXorBuf.rewind();
        for(xx=0; xx<w; xx++) {
            chk = bitStream; // read 1b
            if(packed) chk = rowXorBuf.xorMove(chk); // xor with XOR buffer
            if(chk) {
                if(size == 1) {
                    writePixel(x+xo+xx, y+yo+yy, color);
                } else {
                    writeFillRect(x+(xo16+xx)*size, y+(yo16+yy)*size,
                        size, size, color);
                }
            }
        }
    }
...

class BitStreamsReader {
public:
    BitStreamsReader(uint8_t *arr, unsigned int maxSize) {
        blockSize = maxSize;
        bitMask = 0x80;
        bytePtr = arr;
        zerosCount = 0;
        remainingBits = 0;
        markBitInit();
    }
    operator bool() {
        if(!blockSize) { // unpacked stream
            bool retVal = !!(pgm_read_byte(bytePtr) & bitMask);
            next();
            return retVal;
        } else if(remainingBits) {
            bool retVal = !!(pgm_read_byte(bytePtr) & bitMask);
            next();
            if(!--remainingBits) {
                markBitInit();
            }
            return retVal;
        } else if(zerosCount) {
            if(!--zerosCount) {
                markBitInit();
            }
            return 0;
        }
        return 0; //throw "??";
    }
private:
    void markBitInit() {
        if(blockSize) {
            if(pgm_read_byte(bytePtr) & bitMask) {
                remainingBits = blockSize;
            } else {
                zerosCount = blockSize;
            }
            next();
        }
    }
    void next () {
        if(bitMask == 0x01) {
            bitMask = 0x80;
            bytePtr++;
        } else {
            bitMask >>= 1;
        }
    };
    uint8_t *bytePtr, bitMask, zerosCount, remainingBits, blockSize;
};

class BitXorBuffer {
public:
    BitXorBuffer(int bufSize) {
        bufSize = bufSize/8+!!(bufSize&7);
        bitBuf = new uint8_t[bufSize];
        memset(bitBuf, 0, bufSize);
        rewind();
    }
    ~BitXorBuffer() {
        delete[] bitBuf;
    }
    void rewind() {
        bitMask = 128;
        bytePtr = bitBuf;
    }
    bool xorMove(bool by) {
        if(bitMask & *bytePtr) by ^= true;
        *bytePtr |= bitMask;
        if(!by) {
                *bytePtr ^= bitMask;
        }
        nextBit();
        return by;
    }
private:
    void nextBit() {
        if(bitMask > 1) {
            bitMask >>= 1; // move bit in XOR buffer
        } else {
            bitMask = 128;
            bytePtr++;
        }
    }
    uint8_t *bytePtr, *bitBuf, bitMask;
};

Refactor not to use large ISRs. ISRs should be short and quick. "Big ISR" is a fundmental flaw. You want ISRs to take as few cycles as possible (To the point of not using higher-level Arduino functions like digitalRead(). An ISR should be in-and-out in as few instruction cycles as possible. — Duncan C
– Duncan C, Commented Jul 11, 2019 at 15:11
I have had to do this many times with customers' "code". Step one is to rescue what functionality you can into libraries. Step two is to write a new program using those libraries. — Majenko
– Majenko, Commented Jul 11, 2019 at 15:21
I do, though I prefer not to. I usually work with more powerful systems than Arduino. — Majenko
– Majenko, Commented Jul 11, 2019 at 15:39

chrisl · Accepted Answer · 2019-07-12 07:45:31Z

I will try to answer your question as good as possible, though it is quite broad.

To my knowing there is no ONE design pattern or template, that fit's all cases. But depending on the actual functions of your code you can go different ways using libraries (which are somewhat like templates in this sense).

You can get easy multitasking by using libraries like FreeRTOS. You might even find a compatible version of SafeRTOS, a sibling of FreeRTOS, which has a bigger focus on security. Though the whole RTOS thing might not be right for you and it needs your code to be mostly rewritten/redesigned.

Depending on what ISRs are used and what they are for, you might really need to use interrupts to let the device react immediately. In that case you basically have not choice, but to refactor the whole code as described in the comments (moving as much code as possible to the main loop, making the ISR drastically smaller). For this here some general advice:

First make a graph of the devices function as it should be. Identify code portions that have a common function, but them into blocks, draw arrows between the blocks to show, how they are connected logically. Then identify which portions are really time critical and what the time contrains are (e.g. Do you need the device to react to an event within a few clock cycles or is it ok, if the reaction comes a few us or ms later?). Mark how often the involved events occur at average and at maximum.
Now look at the code blocks directly. Which of them need interrupts to work and what happens with them, if interrupts are turned of during their operation (since - as you already identified - reactivating interrupts inside an ISR can get very messy). For example with interrupts off, Serial prints will not be send directly (because this relies on interrupts). Instead the libraries buffer will be filled. When the buffer is already full, a print will wait for the buffer to empty a big (which does not happen with interrupts off).
Move all code, that is not really time critical, into an if statement in the main loop function. The if statement should be activated by a flag, that would be set in the ISR (be sure to declare it volatile).
Make sure, that there is nothing blocking in your code (no delay(), no waiting for an event in an infinite loop). That also might require serious refactoring, but it's important if you want a fast reaction also outside of the ISR. You might want to look into Finite State Machines for this (they are quite easy to implement and a very powerful construct).
Keep debug prints small, so that it cannot mess up your timing.
Lastly: Make some documentation. That is mostly reglected, because it is not that rewarding, but it can save many hours for the next programmer, that needs to maintain your code.

That might seem very time consuming, difficult and not like the comprehensive guide you were looking for. That's the reason, why maintaining legacy code is mostly a real pain in the a*** and does cost companies a lot of money.

At one point or another the technical debt has to be paid. The inventor of the code clearly didn't (or he/she would't have written an ISR this big), so now you will most likely have to go the hard way.

Problem is also it has 32kB flash, RTOS would probably not work on or is there some tiny version ;-) ? — Jan
– Jan, Commented Jul 12, 2019 at 8:13
The linked site shows this: // Device: loop() -> FreeRTOS | Additional Program Storage // Uno: 444 -> 7018 | 20%, and the Uno has only 32kB flash. Surely it is questionable, if the rest of your code is fitting on top, but that cannot be said without the actual code. — chrisl
– chrisl, Commented Jul 12, 2019 at 8:17
Sure not - not so long code, but there are 24px GFX & compressed fonts yet, so almost whole is full, suppose fonts consume 1/2 - 3/4 of memory. Uncompressed fonts would fill whole mem, compression makes them 1/2, so 16kB without requested GUI ? — Jan
– Jan, Commented Jul 12, 2019 at 8:29
2 compressed GFX fonts used are also in my .Net simulator here github.com/eltomjan/ETEhomeTools/blob/master/ArduinoSimulator/… and github.com/eltomjan/ETEhomeTools/blob/master/ArduinoSimulator/… — Jan
– Jan, Commented Jul 12, 2019 at 9:57
So then you are stuck with refactor the ISR to get it smaller — chrisl
– chrisl, Commented Jul 12, 2019 at 12:39

Stack Exchange Network

Refactoring bad design?

1 Answer 1

Your Answer

Hot Network Questions

Refactoring bad design?

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Related

Hot Network Questions