Design Pattern Advice

Hi All,

I'm in a similar position to:

where I have one large .ino file (2500 lines). Our use case is very similar to as described in the topic above. We are using a BSOM that utilises the following:

  • 1 Connected RFID Reader
  • Realtime clock (yet to be implemented)
  • BLE connectivity
  • Sensors
  • Pin Setup
  • Config
  • Timing

We are a startup and so far our current firmware has been manageable as I am the only developer working on the code. As I am now thinking about hiring additional developers, swapping to a singleton pattern seems like a no brainer. I refactored just over a year ago from a much messier spaghetti setup to the current multiple FSMs setup, but I still can not expect someone to be able to contribute to the codebase effectively. As someone that has never written a c++ class but has somehow managed to create a working firmware with the help of this forum, does anyone have any udemy or youtube suggestions of where to start. Should I just get cracking on one of the above bullet points and go one by one through them or should I just start from scratch and try separate everything out?

Are their any resources within particle that I should be trying to utilise, paid or otherwise? Also over the next few months we will be moving from an inhouse designed PCB to working with an external design house/contract manufacturer. Should I even be considering starting this prior to design house input? Any guidance from someone that has been through this before would be amazing. It seems like both @chipmc and @jgskarda have already been through this.

Thanks in advance.

1 Like

Hi, I would start there. I would isolate just one feature or functionality into a singleton or a class.
It takes time but after that you'll be proud of the spaghetti you left behind.
Best

3 Likes

Also, this guy helped me understand better few c++ intricacies along the years:

3 Likes

Found him linked in the above topic, have been binge-ing it all day. :+1:

1 Like

I went through a very similar process and had a similar limited to no C++ background when I started this Particle.IO adventure about 4-5 years ago. My code base is never done but the Singleton Class structure sure made it much more manageable then the original monolithic code base. Maybe someday like you, an additional developer could read it and understand the madness as well. :slight_smile:

Personally, I watched the Cherno series that @gusgonnet linked above as it fit my learning style and then jumped into it. In 1 solid weekend, I was able to re-structure the code base to the point where it would compile, flash and be functional. Between Cherno's YouTube series and Rick's Singleton class documentation I was able to connect the dots enough to vastly improve the code base and the continued the cleanup from there.

One other aspect that helped my understanding was going through some of Rick's existing libraries base code as many of them use the Singleton design. For example the AB1805 library I was already using and needing to modify so that helped my understanding and modeling some of the code from it.

Below is the structure I ended up with/currently have. Each one being a singleton instance all referenced in Main.cpp.

image

Hope that helps give some inspiration!

3 Likes

Very helpful, thanks all. I will be back when I crash into the 3rd or 4th hurdle. :grinning:

Hi All,

Finally getting some time at this today, and just want to do a quick sanity check that I am approaching this appropriately. So I have just created a blank project.
image

  • config - will setup and control eeprom variables.
  • particle_fn - will setup, define and action any particle functions. There is a function in here that calls and uses methods from the config singleton. Not sure if this is standard/recommended.

config.h

#ifndef __CONFIG_H
#define __CONFIG_H

#include "Particle.h"

/**
 * This class is a singleton; you do not create one as a global, on the stack, or with new.
 * 
 * From global application setup you must call:
 * config::instance().setup();
 * 
 * From global application loop you must call:
 * config::instance().loop();
 */
class config {
public: // Public just for added variables & methods
    
    // EEPROM VARIABLES
    typedef struct{
        uint32_t magic;
        char email[40];
    } eepromConfig;

    eepromConfig config_data;

    void printConfigVariables();  // Log info config varibles to console
    void storeConfigVariables(); // Store variables to config struct

protected: // protected just for added variables & methods
    int m_addr = 100;
    const uint32_t DATA_MAGIC = 0xa2c7206a;
    char init_email[40] = "";

    void loadConfigVariables();

public: // Singleton Boilerplate
    /**
     * @brief Gets the singleton instance of this class, allocating it if necessary
     * 
     * Use config::instance() to instantiate the singleton.
     */
    static config &instance();

    /**
     * @brief Perform setup operations; call this from global application setup()
     * 
     * You typically use config::instance().setup();
     */
    void setup();

    /**
     * @brief Perform application loop operations; call this from global application loop()
     * 
     * You typically use config::instance().loop();
     */
    void loop();

protected: // Singleton Boilerplate
    /**
     * @brief The constructor is protected because the class is a singleton
     * 
     * Use config::instance() to instantiate the singleton.
     */
    config();

    /**
     * @brief The destructor is protected because the class is a singleton and cannot be deleted
     */
    virtual ~config();

    /**
     * This class is a singleton and cannot be copied
     */
    config(const config&) = delete;

    /**
     * This class is a singleton and cannot be copied
     */
    config& operator=(const config&) = delete;

    /**
     * @brief Singleton instance of this class
     * 
     * The object pointer to this class is stored here. It's NULL at system boot.
     */
    static config *_instance;

};
#endif  /* __CONFIG_H */


config.cpp

#include "config.h"

config *config::_instance;

// [static]
config &config::instance() {
    if (!_instance) {
        _instance = new config();
    }
    return *_instance;
}

config::config() {
}

config::~config() {
}

void config::setup() {
    delay(3000);
    loadConfigVariables();
    Serial.println("Config Setup COMPLETE");
}

void config::loop() {
    // Put your code to run during the application thread loop here
}

void config::printConfigVariables(){
        Log.info("addr=%d, email=%s", m_addr, config_data.email);
        Log.info("sizeof(data)=%d", sizeof(config_data));
}  

void config::storeConfigVariables(){
	    EEPROM.put(m_addr, config_data);
}

void config::loadConfigVariables(){
        Log.info("CHECKING EEPROM");
        printConfigVariables();
        EEPROM.get(m_addr, config_data);
        if (config_data.magic != DATA_MAGIC){
            Log.info("MAGIC BYTES DIFFERENT - LOADING NEW DEFAULTS");
            config_data.magic = DATA_MAGIC;
            strcpy(config_data.email, init_email);
            printConfigVariables();
            storeConfigVariables();
        }
        else{
            Log.info("MAGIC BYTES SAME - LOADED EEPROM VALUES");
            Log.info("AFTER EEPROM READ");
            printConfigVariables();
        }
    }

particle_fn.h

#ifndef __PARTICLE_FN_H
#define __PARTICLE_FN_H

#include "Particle.h"
#include "config.h"

/**
 * This class is a singleton; you do not create one as a global, on the stack, or with new.
 * 
 * From global application setup you must call:
 * particle_fn::instance().setup();
 * 
 * From global application loop you must call:
 * particle_fn::instance().loop();
 */
class particle_fn {

protected: // For code added thats not signleton boilerplate
    
    void particleFunctionSetups(); 
    int setEmail(String command);

public:
    /**
     * @brief Gets the singleton instance of this class, allocating it if necessary
     * 
     * Use particle_fn::instance() to instantiate the singleton.
     */
    static particle_fn &instance();

    /**
     * @brief Perform setup operations; call this from global application setup()
     * 
     * You typically use particle_fn::instance().setup();
     */
    void setup();

    /**
     * @brief Perform application loop operations; call this from global application loop()
     * 
     * You typically use particle_fn::instance().loop();
     */
    void loop();


protected:
    /**
     * @brief The constructor is protected because the class is a singleton
     * 
     * Use particle_fn::instance() to instantiate the singleton.
     */
    particle_fn();

    /**
     * @brief The destructor is protected because the class is a singleton and cannot be deleted
     */
    virtual ~particle_fn();

    /**
     * This class is a singleton and cannot be copied
     */
    particle_fn(const particle_fn&) = delete;

    /**
     * This class is a singleton and cannot be copied
     */
    particle_fn& operator=(const particle_fn&) = delete;

    /**
     * @brief Singleton instance of this class
     * 
     * The object pointer to this class is stored here. It's NULL at system boot.
     */
    static particle_fn *_instance;

};
#endif  /* __PARTICLE_FN_H */


particle_fn.cpp

#include "particle_fn.h"

particle_fn *particle_fn::_instance;

// [static]
particle_fn &particle_fn::instance() {
    if (!_instance) {
        _instance = new particle_fn();
    }
    return *_instance;
}

particle_fn::particle_fn() {
}

particle_fn::~particle_fn() {
}

void particle_fn::setup() {
    particle_fn::particleFunctionSetups();
    Serial.println("Particle Function Setup COMPLETE");
}

void particle_fn::loop() {
    // Put your code to run during the application thread loop here
}

void particle_fn::particleFunctionSetups(){
        Particle.function("setEmail_s", &particle_fn::setEmail, this);
    }

int particle_fn::setEmail(String command){
        strcpy(config::instance().config_data.email, command.c_str());
        config::instance().storeConfigVariables(); // This just writes the struct to eeprom
        config::instance().printConfigVariables();
        return 1;
    }

main.ino (labelled as something else)

/*
 * Project particle_device_class_based_refactor
 * Description:
 * Author: Ivan W
 * Date:
 */

#include "config.h"
#include "particle_fn.h"

SerialLogHandler logHandler(LOG_LEVEL_INFO);

// setup() runs once, when the device is first turned on.
void setup() {
  // Put initialization like pinMode and begin functions here.
  System.enableFeature(FEATURE_RESET_INFO);
	Serial.begin(19200);
  config::instance().setup();
  particle_fn::instance().setup();

}

// loop() runs over and over again, as quickly as it can execute.
void loop() {
  // The core of your code will likely live here.
  config::instance().loop();
  particle_fn::instance().loop();
}

So my questions:

  1. Am I going about this the right way?
  2. I am putting some methods into protected (if not required outside of singleton), and some methods into public (if required by other singletons or main.ino)
  3. When should I be considering using a mutex or worker thread? What type of singleton functionality would call for a mutex or worker thread?
  4. If I don't have any code in the singleton loop, ie just using singleton methods when required. Do I need to call the singleton's loop method in my main loop method? eg:
void loop() {
  // The core of your code will likely live here.
  config::instance().loop();
  particle_fn::instance().loop();
}
  1. I was getting an error when trying to setup the particle functions, but fixed based on one of chip's comments from another topic, is the following the correct way to implement particle functions (from particle_fn.h)?
void particleFunctionSetups(){
        Particle.function("setEmail_s", &particle_fn::setEmail, this);
    }

Any advice or criticisms very welcome, I just want to make sure I am not doing anything crazy before ploughing through the remaining 2000+ lines of refactor. Thanks in advance.

3 Likes

@StngBo ,

This looks reasonable to me. Here is how I organized a recent project:

Screenshot 2023-07-10 at 8.41.58 AM

I moved away from using .ino files and am just using .cpp. In my main program "LoRA_Particle_Gateway", I call the setup and loop functions from each of the first line singleton classes and I have my main finite state machine.

My approach has three layers (comments welcome on whether this is the best approach) :slight_smile:

  1. Main program - "LoRA_Particle_Gateway", I call the setup and loop functions from each of the first layer singleton classes and I have my main finite state machine.
  2. First layer classes - Storage, Particle, LoRA, Take Measurements
  3. Second - later classes - FRAM (for storage), RFM-9X (for LoRA), Sensor libraries (for Take Measurements).

The idea - and again - open to input. Is that I might be able to switch my storage device from FRAM to EE Prom to Flash and it should not change anything for the main program as it only sees the stage class. This is, thanks to @rickkas7 StorageHelper.

I have not pulled the trigger on implementing threads - I am letting deviceOS do its thing here. That said, @jgskarda has found a great application of multiple threads to allow the gateway to service both the LoRA radio and the cellular connection back to Particle.

I hope this helped and happy to share more. Thank you for starting this thread, I am sure we will all learn here. My hope is that this modular code will be more maintainable and more of it will be reusable over time.

Chip

3 Likes

I think @chipmc covered most items. A few additional comments:

  1. Am I going about this the right way? - Yes, in my opinion it's a good approach!

  2. I am putting some methods into protected (if not required outside of singleton), and some methods into public (if required by other singletons or main.ino) Correct! I do the same.

  3. When should I be considering using a mutex or worker thread? What type of singleton functionality would call for a mutex or worker thread? As Chip mentioned, I use a separate worker thread specifically to handle LoRa messages from other LoRa nodes. This way I never miss a message from a LoRa node when the main thread is doing other work or is blocked momentarily by something else or the main FSM was in a "connecting to cloud" state where it wasn't setup to service LoRa messages in that state. It took a little bit to figure out how to effectively pass data between the worker thread and main thread. I am still not 100% sure I did it correctly but everything is working properly and I don't miss LoRa messages anymore so must be close enough :slight_smile: I'd say only use the mutex thread if you absolutely have to. Feel free to provide more details here on the application and maybe we can provide guidance.

  4. If I don't have any code in the singleton loop, ie just using singleton methods when required. Do I need to call the singleton's loop method in my main loop method? eg: No you do not. I only call the loop method if there is code that needs to execute every loop. I think I only have 2 maybe 3 of the singleton classes that have code in that singleton's loop so I only have those method calls in my main loop.

  5. I was getting an error when trying to setup the particle functions, but fixed based on one of chip's comments from another topic, is the following the correct way to implement particle functions (from particle_fn.h)? Yes, that is correct. It took me a bit to figure out the right syntax but that matches what I use. One comment I'll make is that all particle functions do not have to be in particle_fn.cpp/.h. I actually use Particle functions in my other singleton classes as well. For example, in my sensors.h/.cpp I have a particle function to individually read each sensor. I just use Particle_fn class for misc. functions that are not found in any specific class. For example PublishVitals, DeepReset. etc.

Hope that helps!

3 Likes

Hi Chip,

Thanks for the detailed overview. I would be very interested to learn more about how your layers work and the approach that you have taken?

Currently, while my monolith app does have FSMs, there is no hierarchy, but abstracting layers along with the splitting of codebase into singletons seems like a great idea.

This is what I am thinking of trying to implement. The graph below shows a simplified version of our application.

image

This would be my top layer FSM. I am now thinking I should have a second layer FSM within the Normal Operation State, which when simplified would look like the following:

Some quirks here, are that I would have to go back to scheduler every loop, to look for interval based events, e.g. command from particle function, service watchdog, check and report vitals, etc. I have to allow these to "interrupt" my event based loop as an event in theory could last for 1 second up to hours.

Similarly I would also have a second layer FSM for the Bluetooth mode. I will also probably need third layer FSMs for the RFID and External Sensors, as these will be powered up at the start of events and then powered down at the end. The RFID reader is a bit more complicated as it toggles on and off based on timers.

Anyway, as always very interested in feedback. Hopefully this discussion will be helpful to people further down the track.

@StngBo ,

This looks like a decent approach. Having FSM's for each of your operating modes is how I do things as well (I have an FSM for LoRa for example that is called from my main program FSM).

I am no expert here as I was introduced to FSM but @gusgonnet on these community forums a couple years ago. I have run into some situations where I need to make the following trade-offs:

  • How much do I do in one state versus making a more complex state
  • How do I keep variables / functions to their minimum "context"
  • Do I pass values in the function call versus having a global structure
  • Do I use a state for scheduling vs using a software timer

I have found these approaches work for me:

  • I try to minimize global variables using instead two strcutures - one for System and one for Current values. I work to keep both of these small.
  • I use the "Idle" state as the default for each FSM and where I "schedule" my activities
  • I try to minimize scope for variables and functions - passing values in a function call if possible
  • If possible, I will use a system timer to keep the FSM from being stuck in a "WAIT_FOR_XXX" state too long or to make a state that does nothing but wait. Most waiting is done in "IDLE".

Not sure if this is helpful and would appreciate your or the communities' thoughts as well.

Chip

2 Likes

Hey Ivan!

from your drawings I am led to think that they depend on each other.
Maybe this can get in the way. In general, if you can think of them of being independent, it'll be easier to code (I find).

For instance, the bluetooth mode or the normal operation modes, can they run in parallel? Are they controlled by the Scheduler? Must they be controlled by the scheduler?

All questions that you can ponder since I have no visibility in your intentions.
Best

this one is a tough one. I read a book once about FSMs where the fsm needed to count 1 million events, so the author went to the extreme of suggesting AGAINST creating one state per count, you have a counting state and then a local variable in it. This is an extreme, and usually we find things like this in a much subtle "scale" that does not help to decide. Dor instance, if it needs to count to 2, do you still use a counter variable or you simply add a state to the fsm?

I try to stay away from timers, so to avoid the thread issues they may create (or the need to use an atomic variable for these cases). If I can do it from loop(), I choose that route almost always.
I only use timers for debouncing. Often times, if the temperature sensor is read at 3.5 seconds vs at 3 seconds, does not impact the result (but in some cases it might).
So if the scenario allows for some room, I code in loop, or in the FSM's/singleton class loop().

Cheers

I tend not to see it that way, in general if a state needs to wait for something to happen, I let it be and make that state wait for that event, event if the event takes a long time to come.

1 Like

@gusgonnet ,

Thanks for that perspective. You have more experience in this area so, I need to take another look when I next re-open my code.

Chip

Hey Gus,

Thanks for the response, Bluetooth and Normal operation can't run in parallel. If a BLE device connects to our device then when the current event is finished, the device will go into BLE mode.

Now BLE mode is very similar to the normal operation, they both look for data from the RFID and sensors, but it puts the sensors and RFID reader on 100% of the time while connected.

I guess the scheduler is most like Chips Idle state. It just passes through, but will respond to any particle functions that have been called or if a watchdog check-in is required. But the Scheduler does decide whether to put the device into normal operation or BLE mode.

Thanks for the feedback

Hi Chip,

Regarding your two structs for global usage. Do you have these defined in their own singleton? I realise this is probably just a design decision.

I like the idea of having all my globals in one location, but I realise I should isolate globals to individual classes if they are only required in that class. Is there a memory overhead with keeping all globals in a struct versus individual variables? Just interested why you chose that.

Thanks for your input so far.

@StngBo ,

Yes, these structures are defined in Singletons using The StorageHelper library. They, in turn, call the Singleton for my persistent storage technology - FRAM. The cool thing about this is that you could switch to using Flash for example without changing anything in you code that accesses these variables.

You can see how I do this in my gateway repo if you are interested (see the MyPersistentData files).

My understanding is that these variables are stored in the persistent storage not in the Boron's working memory with a small cache that can be cleared manually or will be flushed every second or so. I believe this is a more efficient approach as your data is stored to persistent memory and you are not keeping all these variables in memory.

Did this answer your question?

Chip

1 Like

Very helpful, will have a root around your LoRA git. Thanks a million

@all,

This is the only Singleton thread that is currently open so I thought I might ask my basic (sorry!) question here.

I would like to keep the number of "products" and code bases to a minimum. So, I try to abstract the code from the sensors. For example, the code for my car counter and a tipping rain gauge are exactly the same. So, I can have one "product" and one code base to maintain and simply change the reporting of "cars" and "rainfall" on the back-end system - Ubidots. There is a configuration variable "sensor type" that you can set with 1 as a car counter and 2 as a rain gauge so the back-end can easily change the units.

It gets a little trickier if different sensors require different libraries or functions / classes for configuration. It would seem to be inefficient if you had to include all the libraries and classes for all the potential sensors when each device will only use one or two. So, I have a few questions:

  1. I believe I had heard that you can #include a library but if you don't end up calling any of its functions, the compiler does not include it in the code - is this right?

  2. Would the same be true for Singletons / Classes?

  3. If not, is there a pre-compiler trick - #IFDEF ?

Any advice would be appreciated.

Thanks, Chip