0

In my simple Arduino project, in order to keep things tidy, I decided to create a "Mode Manager" that will handle passing between one mode and another. The basic concept is that each time I want to change the mode, then it will instantiate the next mode and replace the previous one.

Please note that I have 12+ years experience in OOP and Java / Scala, but no experience whatsoever in C++, just the stuff needed to make an Arduino do its stuff without too many structures.

With some studying I managed to create the following "interface" structure:

File: modes/ModeManager.h. This should keep a reference to the current mode, and delegate to it the looping and instantiating the next mode (each mode will know which is the next in line)

#ifndef __MODE_MANAGER__
#define __MODE_MANAGER__

#include <stdint.h>
#include "modes/Mode.h"

class ModeManager {
 private:
  Mode *mode;

 public:
  ModeManager();
  ~ModeManager() {};
  virtual void loop(uint8_t voltage);
};

#endif

File: modes/Mode.h is the actual "mode" that runs the real loop function of the program. It also handles instantiating the next mode

#ifndef __MODE__
#define __MODE__

#include <stdint.h>

class Mode {
 public:
  Mode() {}
  virtual ~Mode() {}
  virtual void loop(uint8_t voltage) = 0;
  virtual void getNext(Mode * target) = 0;
};

#endif

File: modes/ModeManager.cpp

#include "ModeManager.h"

#include <stdint.h>
#include <Arduino.h>
#include "modes/Mode.h"
#include "modes/DummyMode.h"
#define VOLTAGE_NEXT_MODE 5

ModeManager::ModeManager() {
  DummyMode cmode = DummyMode();
  mode = & cmode; // I'm not sure how this should actually be done
}

void ModeManager::loop(uint8_t voltage) {
  if (voltage == VOLTAGE_NEXT_MODE) {
    Serial.println("Switching to next mode");
    mode->getNext(mode);
  } else {
    Serial.println("Calling mode loop");
    mode->loop(voltage); // From here, nothing.
  }
}

File: modes/DummyMode.h

#ifndef __DUMMY_MODE__
#define __DUMMY_MODE__

#include "modes/Mode.h"

class DummyMode : public Mode {
 public:
  DummyMode();
  ~DummyMode() {}
  virtual void loop(uint8_t voltage);
  virtual void getNext(Mode * target);
};

#endif

File: modes/DummyMode.cpp

#include "modes/DummyMode.h"

#include <Arduino.h>

DummyMode::DummyMode() {
  Serial.println("Initialization of dummy mode");
}

void DummyMode::loop(uint8_t voltage) {
  Serial.print("Voltage: ");
  Serial.println(voltage);
}

void DummyMode::getNext(Mode * target) {
  DummyMode nextMode = DummyMode();
  target = &nextMode;
}

and finally my main.cpp

#include <Arduino.h>

#include "modes/ModeManager.h"
#include "modules/memory.h"
#include "modules/monitor.h"

ModeManager * modeManager;

void setup() {
  pinMode(A0, INPUT);
  Serial.begin(9600);
  Serial.println("Init");
  ModeManager mm = ModeManager();
  modeManager = & mm;
}

void loop(void) {
  uint8_t voltage = map(analogRead(A0), 0, 1024, 0, 5);
  modeManager->loop(voltage);
}

Now, theoretically I see no reason why this should not work. In practice I'm 99.9% sure I am doing something wrong with initializations and pointers. When I try to run this code, I get the following serial out:

Init
Initialization of dummy mode
Calling mode loop

Which means that it freezes at the first iteration of the loop, right before calling mode->loop(voltage);

Can anyone point me in the right direction? Again, I have really no experience of C++, and my knowledge of how to make this structure came from various online resources of C++ programming, including some answers here so please bear with me

Luca
  • 1,116
  • 2
  • 15
  • 24
  • 1
    `DummyMode nextMode = DummyMode();` creates a local variable on the stack that is destroyed at the end of the function, and you're returning a pointer to this stack space. – Mooing Duck Feb 18 '21 at 16:43
  • 1
    Alll your use of `&` is wrong `target = &nextMode;` `modeManager = & mm;` you hold on to pointers to objects that no longer exist after the scope ends. – drescherjm Feb 18 '21 at 16:43
  • 1
    You're going to have a really hard time writing C++ if you treat it like a mix of C and Java. It isn't either of those things, and is complex enough to warrant reading a [book](https://stackoverflow.com/q/388242/212858). I'm not going to attempt an answer, because there are _so many_ problems in this code it will turn into a code review, and that's a different site. – Useless Feb 18 '21 at 17:15

1 Answers1

2

Here:

DummyMode cmode = DummyMode();

you are creating a DummyMode instance cmode with automatic lifetime (commonly referred to as on the stack).

You then take the address and assign that to another variable:

mode = & cmode; // I'm not sure how this should actually be done

Since cmode is an object with automatic lifetime it will be destroyed when the program leaves the scope in which it was created. As a result the pointer stored in mode will be dangling and referring to an object that no longer exists. Dereferencing it will trigger undefined behaviour.

It looks like your intention is to create the object with dynamic lifetime. You can do that with the following:

mode = new DummyMode();

Though then you are responsible for ensure the object is destroyed when it is no longer needed. That would be done with delete:

delete mode;

In more idiomatic/modern C++ it is typically to use smart pointers to manage the lifetime of such objects and not manually call delete. I see that C++'s standard library is not available for Arduino, but there does seem to be a smart pointer available: https://www.arduino.cc/reference/en/libraries/arxsmartptr/ I would recommend having a good look at using that to have a much easier time avoiding memory leaks.

PeterSW
  • 4,921
  • 1
  • 24
  • 35
  • Using the `new` keyword worked like a charm. I converted the `getNext` functions to `Mode * getNext();` to return the new pointer and allow myself to destroy the previous instance. I will have a look at the smart pointers when I got the chance, but since this is a small project, with no real optimization need, this is the fastest solution at the time! Thanks! – Luca Feb 18 '21 at 17:51
  • 2
    Smart pointers aren't an optimization for speed, they're an optimization for ease of use and correctness. – Useless Feb 18 '21 at 17:52