C++ referencing instances created within a function's scope

Question

Context

The context of the problem is that I am currently writing a small library for use with the Arduino in order to act as a game controller. The problem I am encountering has more to do with C++ than anything Arduino specific however.

I've included the libraries' header and source code below, followed by the Arduino code. I've truncated it where possible.

Problem

In short, only the last switch / action I define actually gets properly handles.

These actions get defined in the Arduino setup function. For example:

controller.addSwitchContinuous(10, 0);  // Pin 10; btn index 0

means that pin 10 gets mapped to button 0. When pin 10 is switched closed this is treated as the button being pressed. This works fine for a single action but when I start adding more only the last action actually works. So in the following example only pin 9 is recognized:

controller.addSwitchContinuous(10, 0);  // <-- Doesn't work
controller.addSwitchContinuous(9, 1);   // <-- Works

This goes for any arbitrary number of actions:

controller.addSwitchContinuous(10, 0);  // <-- Doesn't work
controller.addSwitchContinuous(9, 1);   // <-- Doesn't work
controller.addSwitchContinuous(8, 2);   // <-- Doesn't work
controller.addSwitchContinuous(7, 3);   // <-- Works

Potential causes

I am fairly novice with C++ so this I suspect I'm doing something wrong with pointers. More specifically, something seems wrong with how the Joystick_ instance gets passed around.

I have been fiddling with the constructor and trying to use references instead of pointers but I couldn't get it to work properly.

I can confirm the iteration in JFSF::loop does iterate over all actions, if I modify it with:

void JFSF::loop()
    {
        for (int n = 0; n < _nextActionIndex; n++)
        {
            if (_actions[n])
            {
                _actions[n]->loop();
                _joystick->setButton(n, PRESSED);  // Debug: Set button pressed, regardless of switch.
            }
        }
        if (_doSendState)
        {
            _joystick->sendState();
        }
    }

then buttons 0 through n get pressed as expected. It is possible that loop() isn't properly being called, but I would expect it to fail for the N = 1 case as well in that case. Furthermore the fact the last action always succeeds would suggest the iteration is ok.

Full code

// JFSF.h
#ifndef JFSF_h
#define JFSF_h

// ... include for Arduino.h and Joystick.h; bunch of defines

namespace JFSF_PRIV
{
class AbstractAction
{
public:
  virtual void loop();
};

/* A Switch that essentially acts as a push button. */
class SwitchContinuousAction : public AbstractAction
{
public:
  SwitchContinuousAction(Joystick_ *joystick, int pin, int btnIndex);
  void loop();

private:
  Joystick_ *_joystick;
  int _pin;
  int _btnIndex;
};

} // namespace JFSF_PRIV

class JFSF
{
public:
  JFSF(Joystick_ *joystick, bool doSendState); // doSendState should be true if Joystick_ does not auto send state.
  void loop();
  void addSwitchContinuous(int inputPin, int btnIndex);

private:
  Joystick_ *_joystick;
  JFSF_PRIV::AbstractAction *_actions[MAX_ACTIONS];
  int _nextActionIndex;
  bool _doSendState;
};

#endif

Source file (trimmed):

// JFSF.cpp
#include "Arduino.h"
#include "Joystick.h"
#include "JFSF.h"

#define PRESSED 1
#define RELEASED 0

// Private classes
namespace JFSF_PRIV
{

SwitchContinuousAction::SwitchContinuousAction(Joystick_ *joystick, int pin, int btnIndex)
{
    _joystick = joystick;
    _pin = pin;
    _btnIndex = btnIndex;
    pinMode(_pin, INPUT_PULLUP);
}

void SwitchContinuousAction::loop()
{
    int _state = digitalRead(_pin) == LOW ? PRESSED : RELEASED;
    _joystick->setButton(_btnIndex, _state);
}

} // namespace JFSF_PRIV

JFSF::JFSF(Joystick_ *joystick, bool doSendState)
{
    _joystick = joystick;
    _nextActionIndex = 0;
    _doSendState = doSendState;
}

void JFSF::addSwitchContinuous(int inputPin, int btnIndex)
{
    JFSF_PRIV::SwitchContinuousAction newBtnAction(_joystick, inputPin, btnIndex);
    _actions[_nextActionIndex++] = &newBtnAction;
}

void JFSF::loop()
{
    for (int n = 0; n < _nextActionIndex; n++)
    {
        if (_actions[n])
        {
            _actions[n]->loop();
        }
    }
    if (_doSendState)
    {
        _joystick->sendState();
    }
}

For completeness sake, this is the code for the Arduino, but it is pretty much just declarations:

#include <JFSF.h>

// ... A bunch of const declarations used below. These are pretty self explanatory.

// See: https://github.com/MHeironimus/ArduinoJoystickLibrary#joystick-library-api
Joystick_ joystick(HID_REPORT_ID,
  JOYSTICK_TYPE_JOYSTICK, // _JOYSTICK, _GAMEPAD or _MULTI_AXIS
  BTN_COUNT, HAT_SWITCH_COUNT,
  INCLUDE_X_AXIS, INCLUDE_Y_AXIS, INCLUDE_Z_AXIS, 
  INCLUDE_RX_AXIS, INCLUDE_RY_AXIS, INCLUDE_RZ_AXIS,
  INCLUDE_RUDDER, INCLUDE_THROTTLE, 
  INCLUDE_ACCELERATOR, INCLUDE_BRAKE, INCLUDE_STEERING);

JFSF controller(&joystick, !DO_AUTO_SEND_STATE);

void setup() {
  joystick.begin(DO_AUTO_SEND_STATE);
  controller.addSwitchContinuous(10, 0);  // <-- Doesn't work
  controller.addSwitchContinuous(9, 1);   // <-- Works
}

void loop() {
  controller.loop();
}

References

ArduinoJoystickLibrary (Source for Joystick_) can be found here: https://github.com/MHeironimus/ArduinoJoystickLibrary#joystick-library-api

you should post a [MCVE](https://stackoverflow.com/help/mcve) (especially focus on the M part) — m.s., Dec 12 '18 at 22:10
I'll try to recreate the problem in a C++ console application and update the post. If you have suggestions for the title I'll happily change it to something more constructive. — ricekab, Dec 12 '18 at 22:14
`JFSF_PRIV::SwitchContinuousAction newBtnAction(_joystick, inputPin, btnIndex);` then `_actions[_nextActionIndex++] = &newBtnAction;` is a common mistake. You can't store the address of a temporary and use it later. `newBtnAction` no longer exists after the function ends making the pointer invalid / point to random garbage memory. This is a case of Undefined Behavior. Unluckily it worked for some of your cases. — drescherjm, Dec 12 '18 at 22:14
The problem is essentially the same as this: https://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope/6445794#6445794 — drescherjm, Dec 12 '18 at 22:18
Thanks, that makes sense and would correspond with the problem I'm having. I'll post a fix once I figured it out. I'll update the title to be more descriptive as well. — ricekab, Dec 12 '18 at 22:23
"I'll post a fix once I figured it out" ... you can answer your own question if you like, but please dont edit your question to fix errors in the code! — 463035818_is_not_an_ai, Dec 12 '18 at 22:33
also your "Update 0" does not belong in the question. Comments are there to be deleted at some point and your question is rather pointless when it starts with the answer before saying what is the problem ;) — 463035818_is_not_an_ai, Dec 12 '18 at 22:36
Right, I don't know exactly what the policy is here. The problem is that the problem description is kind of the answer as well since my original question was simply wrong. — ricekab, Dec 12 '18 at 22:40
***Right, I don't know exactly what the policy is here*** StackOverflow is a Q&A site with the primary goal to help future readers with the same problem. The question should not contain the answer. I think with my edit I fixed it in a reasonable manner. — drescherjm, Dec 12 '18 at 22:57
I've included my code change in a separate answer as well just in case someone might find it useful. I'll leave @user463035818 's answer as the accepted one since it's more clear on what the core of the issue is. — ricekab, Dec 12 '18 at 23:26

score 5 · Accepted Answer · answered Dec 12 '18 at 22:25

5

I dont really understand your code. Please read How to create a Minimal, Complete and Verifiable example. Anyhow, the following is certainly wrong and likely the cause of your problem:

void JFSF::addSwitchContinuous(int inputPin, int btnIndex)
{
    JFSF_PRIV::SwitchContinuousAction newBtnAction(_joystick, inputPin, btnIndex);
    _actions[_nextActionIndex++] = &newBtnAction;
}

Lets rewrite it a bit for clarity:

void foo(){
    T bar;
    container[index] = &bar;
}

What happens here is that bar gets destroyed when it goes out of scope, hence the pointer you put into the container, points to garbage. Presumably somewhere else in your code you are dereferencing those pointers, which is undefined behaviour (aka anything can happen).

Long story short: It is a common pattern among c++ beginners to overuse pointers. Most likely you should make container a container of objects rather than pointers and make use of automatic memory managment instead of trying to fight it.

answered Dec 12 '18 at 22:25

463035818_is_not_an_ai

109,796
11
89
185

I recall I started with that approach but couldn't figure out how to store an array of objects using a common abstract interface (with the shared `void loop()` signature). I'll have to go over some tutorials. Thanks for the help. – ricekab Dec 12 '18 at 22:37
1

@RiceKab ok, sorry missed that. Still no reason to get your hands dirty. In that case you store smart pointers in the container and they'll do the clean up for you – 463035818_is_not_an_ai Dec 12 '18 at 22:39
1

@RiceKab `std::unique_ptr` and [`std::make_unique`](https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique) are your best friends when faced with a problem like this. – user4581301 Dec 12 '18 at 22:41
My prior experience with C++ did involve using the standard library. Unfortunately the Arduino has some severe storage and memory constraints. I do know there are some (partial) ports available so that might be worth looking into. – ricekab Dec 12 '18 at 22:48
I "fixed" it by just moving the instance creation up to the global scope. Given the context of the application this seems like an acceptable solution. I included the code change in a separate answer below. Thanks again for the help :) – ricekab Dec 12 '18 at 23:28

score 1 · Answer 2 · answered Dec 12 '18 at 23:23

Thanks to @user463035818 and @drescherjm for identifiying the actual problem.

So in the end I fixed it by simply moving the Action object creation up to the Arduino code (where it's essentially global) and passing references to those objects to the controller.

In code this translates to:

JFSF.cpp

void JFSF::addAction(JFSF_PRIV::AbstractAction *action){
    _actions[_nextActionIndex++] = action;
}

Arduino code (ino)

// See code in original post

JFSF controller(&joystick, !DO_AUTO_SEND_STATE);

JFSF_PRIV::SwitchContinuousAction btnOne(&joystick, 10, 0);
JFSF_PRIV::SwitchContinuousAction btnTwo(&joystick, 9, 1);

void setup() {
  joystick.begin(DO_AUTO_SEND_STATE);
  //  controller.addSwitchContinuous(10, 0);  // Pin 10; btn index 0
  //  controller.addSwitchContinuous(9, 1);   // Pin 9 ; btn index 1 
  controller.addAction(&btnOne);
  controller.addAction(&btnTwo);
}
// loop() is unchanged

C++ referencing instances created within a function's scope

2 Answers2