Help me untangle this bit of code!

Hey, everyone!

I’m making a basic menu system for my game. It’ll store each menu item’s type, (button or checkboxes, and a few other cosmetic things) X and Y coordinates, and a pointer. Here’s my code so far.

– Edit –
more context for that “a pointer” bit:
Based on the type of menu item, it’ll change what type of pointer it is.
If it’s a button/invertedButton, it’ll be a function pointer in ROM.
If it’s a checkbox, it’ll be a boolean variable pointer in RAM.
– End edit –

I have three questions:

  1. How do I fix all these errors and warnings?
  2. Can I have a pointer that points to either RAM or ROM based on the menu item’s type …or am I sorely mistaken?
  3. I really can’t wrap my head around pointers. I get that they’re positions in memory and all that stuff, but I don’t get all the weird *s and &s and ->s that show up in modern C++. Can someone give me some pointers on this?

Can I suggest you put the whole code up into GitHub (or whatever) so we can see ’ all these errors and warnings’?

I see what you did there. http://www.cplusplus.com/doc/tutorial/pointers/

Are you looking for a C or C++ (OOP) way to do it?

If you wanna go the C++ way, you can create a MenuItem class, then create subclasses for various types of buttons, like BooleanMenuItem, FunctionMenuItem etc. They can all have a virtual method clicked that will do different things in different subclasses. @Pharap will probably give you more ideas here.

Also you may want to take a look at unions.

Anyway, if your menu is really simple (just a bunch of items without submenus) I wouldn’t complicate it too much, I’d simply create an array of strings, then check which one was clicked in a switch. Sometimes simples is better.

1 Like

They’re actually all inherited from C and haven’t changed in decades.

The only thing C++ did in terms of pointers was to add references (another use of &).

Pun intended? :P

* can either be used to create a pointer type or to dereference a pointer.
For example T * x; means that x is a pointer to an object of type T,
and then *x would ‘dereference’ x to get the value of the object being pointed to.
(Technically it gets a reference to the object pointed to, but I don’t want to complicate matters if you don’t understand references.)

& takes the address of a variable and returns a pointer.
E.g. T * x = &y; would mean that x is a pointer to an object of type T and the object it points to is y. &y alone means 'the address of y'.

-> dereferences the pointer on the left hand side and then acts as a . on the object the pointer was pointing to.
In other words x->y is translated to (*x).y.
In fact (*x).y will work perfectly. -> is just a shorthand.

All of these explanations also apply to C as far as I’m aware because they were inherited from C.

On the Arduboy (and any code using Arduino or avr-libc),
reading from progmem can’t be done through a normal dereference.
You have to use pgm_read_byte or one of the variants.
For reading whole objects, you’re best off using memcpy_P.
For reading pointers use pgm_read_ptr and static_cast the result (e.g. static_cast<const char *>(pgm_read_ptr(stringTable))).

On the Arduboy a pointer can point to RAM, ROM (progmem) or even EEPROM, there’s no way of knowing which just by looking at the pointer, which is why you have to be careful about how you use them and why the F macro exists (to tell print/println that “this is a pointer to a string in progmem, not a string in RAM”).

So yes, it’s possible, but you have to be careful about what you’re doing.
If you try to read a progmem pointer as a RAM pointer or a RAM pointer as a progmem pointer then you’ll end up reading the wrong thing and the result could be disasterous.

(Disasterous as in your program breaks and does weird things, not disasterous as in your Arduboy gets destroyed. It should be almost impossible to brick your Arduboy by writing buggy code.)

You haven’t given us enough information to solve all your problems, but I’ve tried to solve what I can…

I started trying to fix your problems, but soon discovered that there isn’t actually enough code here to figure out how to fix your errors.

Over half the errors are "x was not declared in this scope",
which means that it can’t compile because it needs some of the things you’ve left out:

  • MENU_OTHER
  • menuItem_FuncPointer
  • boolPointer1
  • boolPointer2
  • TO_
  • ButtonState
  • ab
  • BLACK
  • WHITE

What little I can tell you about what’s wrong with it (just from reading the error list, so this may not be all the problems):

Your first problem is that you’re using ... outside a comment (here and here to be precise).
... does actually have a meaning in C++, but only in very specific contexts, hence this is upsetting the compiler.

Secondly, MENU_ITEMS_LOOKUP is supposed to be an array of const MenuItemInfo *, but you’re trying to initialise the pointers with {MENU_ITEMS_MAIN, 4}.
In other words, you’re trying to initialise pointers to objects with the objects themselves.
Presumably MENU_ITEMS_LOOKUP should instead be an array of const MenuItemInfo.

Your third problem is with (*(menuItems[index].actionPointer))().
actionPointer is a void *, not a function pointer, so you can’t try to call it.
In fact, the only things you can do with a void * is reassign it or cast it to another type.
Most of the time resorting to void * is a bad idea because it removes important type information and is often a sign of a bad design.

Reading the comment on actionPointer (Can be either a ROM function pointer or a RAM variable pointer)…
@drummyfish’s suggestion of using a union is possibly a better option in this case,
but I can’t really offer up a solution without knowing more.
Specifically I’d need to know:

  • What types of things actionPointer would be pointing to
  • Whether the type of object actionPointer points to depends soleley on the value of MenuItem::type or whether there’s more to it

I’m very concerned about what TO_ is defined as.
I worry in general when I find macros cropping up in code because they generally aren’t needed and should be avoided like the plague for anything other than conditional compilation.

While I think of it, don’t use ‘macro case’ (all caps and underscores) for anything other than macros, it’s considered bad practice in C++.

By the way, if you want I can show you a way to make it possible to write:

const MenuItemInfo MENU_ITEMS_LOOKUP[]
{
	MENU_ITEMS_MAIN,
	MENU_ITEMS_MODE,
	MENU_ITEMS_MODE_A,
	MENU_ITEMS_MODE_B,
	MENU_ITEMS_OPTIONS
};

And have the length automatically inferred by the compiler.
It requires using a (fairly simple) template function though,
so I’d need to explain how it works.


I usually avoid cplusplus.com because it’s been known to get important things wrong before (mainly in the documentation of the standard library, but also misusing terminology), but that particular part of the tutorial looks fine for an intro at least.

It does some stuff I don't like though:
  • Declaring local variables before they’re used
  • Not putting spaces around binary operators
  • Redundant brackets on return values
    • E.g. return (g); should simply be return g;
  • Using function pointers ‘the long way’
    • E.g. (*functocall)(x, y) instead of just functocall(x, y)
  • Bad variable names
    • E.g. functocall instead of functionToCall or just function
    • psize instead of just size
  • using namespace std; - don’t do this!

I’d like to also offer up this tutorial though.

Specifically these sections:


Actually that might not be the best way.
If this were desktop programming then sure, that’s how UI stuff is often done,
but virtrual functions chew up a fair bit of memory on Arduboy.

As I said earlier in my comment, unions could be an option,
but whether they’re the best option depends on how they’re used and what they’re used for.

And they absolutely must have some way of knowing which member of the union is the active member (e.g. through enum-based tagging), otherwise you’re looking at undefined behaviour.

Two possible problems with that.

Firstly switches become horrible to maintain after a few items.
A progmem array oriented approach makes the code more data driven and allows the menu definitions to be spread across separate files instead of squashed into one big monolith.

Secondly, they can often end up using more memory than a hand-crafted progmem manipulation approach.
The compiler will sometimes turn the code into a jump table,
but I’ve found that it doesn’t do that nearly as much as it should do.

switches are only a reasonable option if there are only a few menus and the logic in them is quite simple.

2 Likes
Old post draft (horrible and messy)
Show Missing Code

Ah, I forgot that I might need to actually include more than one file of my code. Sorry about that.
Here’s what most of those errors refer to:

#include <Arduboy2.h>
#include <ArduboyTones.h>
#include <Tinyfont.h>
#include <EEPROM.h>

#define TO_INT(x) static_cast<uint8_t>(x)
#define TO_(T, x) static_cast<T>(x)

Arduboy2 ab;
ArduboyTones at = ArduboyTones(ab.audio.enabled);
Tinyfont tf = Tinyfont(ab.sBuffer, Arduboy2::width(), Arduboy2::height());

bool redrawMenu = false;

void setup() {
	ab.begin();
	ab.setFrameRate(60);
	ab.clear();
	
	loadMenuItems(0);
}

void loop() {
	// todo: draw options
}

and stuff to make the menu work:

const char MENU_OTHER[] PROGMEM = "???"; // placeholder

enum class ButtonState : uint8_t {
	Unselected,
	Selected,
	Pressed
};

The entire '...' business was just me trimming some irrelevant stuff from the file and forgetting how to format things.

TO_ is just my lazy way of writing static_cast<T>(v) as TO_(T, v). It’s horrible, but I like it and it looks more like a regular cast.

But I’ve been pretty scared of using classes for my Arduboy projects. Is there any significant overhead with classes, or will it all compile away? off topic


Clearing things up

To clear things up, this is all I want the menu system to do:

  1. Store a bunch of menu options in a decently compact format.
  2. Have two types of menu options: buttons and checkboxes.

When a menu needs to be shown…

  1. Copy the menu options to RAM.
  2. Draw them.
  3. Go into a loop of checking buttons and moving a cursor around.

When an option is clicked…

  1. Check if it’s a button. If so, use the pointer as a function pointer.
  2. Otherwise, if it’s a checkbox, use the pointer as a pointer to a boolean value that will get toggled.

Also, some good news: my code now compiles! I used the unions that @drummyfish was talking about, and they work really nicely! Now the menu definitions look more like this:

enum class MenuItemType : uint8_t {
	Button,
	InvertedButton,
	Checkbox,
	Info
};

union MenuItemPointer {
	void (*functionPointer)();
	bool* booleanPointer;
};

struct MenuItem {
	uint8_t x;
	uint8_t y;
	MenuItemType type;
	const char* textPointer;
	MenuItemPointer actionPointer;
};
struct MenuItemInfo {
	const MenuItem* items;
	uint8_t length;
};

const MenuItem MENU_ITEMS_MAIN[] PROGMEM {
	{14, 26, MenuItemType::Button, MENU_OTHER, {.functionPointer = menuItem_Main_Play}},
	{66, 26, MenuItemType::Button, MENU_OTHER, {.functionPointer = menuItem_Main_Play}},
	{14, 42, MenuItemType::Button, MENU_OTHER, {.functionPointer = menuItem_Main_Play}},
	{66, 42, MenuItemType::Button, MENU_OTHER, {.functionPointer = menuItem_Main_Play}}
};
// and so on, until
const MenuItem MENU_ITEMS_OPTIONS[] PROGMEM {
	{14, 18, MenuItemType::InvertedButton, MENU_BACK, {.functionPointer = menuItem_Main_Play}},
	{66, 18, MenuItemType::Checkbox, MENU_OTHER, {.booleanPointer = &optionsBool1}},
	{14, 34, MenuItemType::Checkbox, MENU_OTHER, {.booleanPointer = &optionsBool2}},
	{66, 34, MenuItemType::Button, MENU_OTHER, {.functionPointer = menuItem_Main_Play}}
};

My only concern right now is that

void loadMenuItems(uint8_t index) {
	menuLength = pgm_read_byte(&(MENU_ITEMS_LOOKUP[index].length));
	memcpy_P(menuItems, &(MENU_ITEMS_LOOKUP[index].items), sizeof(MenuItem) * menuLength);
}

doesn’t work.


After so, so long. I found out why this wasn't working. It's because I forgot one (1) PROGMEM declaration. Fixed it and it works. AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAaaaa, why am I majoring in computer science

Still: thanks for all the help and resources, they’ll help me better understand pointers in the future.

1 Like

I bet your computer science course isn’t touching on C++ or even Arduino / Arduboy programming. You are probably flying high with high-level languages and frameworks.

2 Likes

A few comments about the code…

If you don’t like typing, you’re in the wrong profession. :P

More seriously, just get used to static_cast, you’ll have less questions to answer and less complaints to deal with.
Defining a macro to act as a cast just because it’s less typing is only a step up from daft hacks like #define begin { #define end } or #define forever for(;;).

That’s non-standard syntax for the record.
It’ll be allowed in C++20, but until then it’s non-standard.

Be really careful with unions.
If you read to a member other than the one that was last assigned to then you’ve got ‘undefined behaviour’, which means anything could happen.

More practically, in your case you have to be very careful to make sure that when you’re accessing functionPointer it is actually a pointer to a proper function.
If you read functionPointer when it was actually booleanPointer that was last set then you could end up running code in the middle of an unrelated function which could have almost any result - for example, it could unbalance the stack or scramble the display buffer, and it could do almost anything to the hardware (overwrite eeprom, play noise out the speakers).

To be safe, you might want to make the actionPointer private and provide an accessor function that checks type before it tries to access actionPointer and refuses to touch it if type isn’t the right type. Similarly it would be best to introduce some constructors or ‘factory’ functions to make sure that type and actionPointer always match up properly.

E.g. something like this would help prevent mistakes:

// Friendly type alias to overcome the cumbersome function pointer syntax
using Action = void (*)();

union MenuItemPointer
{
	Action function;
	bool * boolean;

	constexpr MenuItemPointer(Action & function) :
		function{function)
	{
	}
	
	constexpr MenuItemPointer(bool & value) :
		boolean{&value}
	{
	}
};

class MenuItem
{
public:
	uint8_t x;
	uint8_t y;
	
private:
	MenuItemType type;
	const char* text;
	MenuItemPointer actionData;
	
private:
	constexpr MenuItem(uint8_t x, uint8_t y, MenuItemType type, const char * text, Action function) :
		x{x}, y{y}, type{type}, text{text}, actionData{function}
	{
	}
	
	constexpr MenuItem(uint8_t x, uint8_t y, MenuItemType type, const char * text, bool & value) :
		x{x}, y{y}, type{type}, text{text}, actionData{value}
	{
	}
	
public:
	static constexpr MenuItem createButton(uint8_t x, uint8_t y, const char * text, Action function)
	{
		return { x, y, MenuItemType::Button, text, function };
	}
	
	static constexpr MenuItem createInvertedButton(uint8_t x, uint8_t y, const char * text, Action function)
	{
		return { x, y, MenuItemType::InvertedButton, text, function };
	}
	
	static constexpr MenuItem createCheckbox(uint8_t x, uint8_t y, const char * text, bool & value)
	{
		return { x, y, MenuItemType::Checkbox, text, value };
	}
};

(Also avoid using ALL_CAPS for non-macros.)


I have to answer this to dispel the FUD.
The answer is no. No no no no no, a thousand times no.

This:

class Point
{
public:
	int x;
	int y;
};

Point point { 5, 6 };

Compiles down to the exact same code as this does:

int x = 5;
int y = 6;

The size of an object is equal to the sum of the size of its parts.

Also, in case you think struct is different: struct is just another word for class.
The only difference is that struct members are implicitly public and class members are implicitly private.

Member functions also add no extra overhead.
This:

struct Vector
{
	int x;
	int y;
	
	int getSquareMagnitude() const
	{
		return ((this->x * this->x) + (this->y * this->y));
	}
}:

Is effectively equivalent to this:

struct Vector
{
	int x;
	int y;
}:
	
int getSquareMagnitude(const Vector * vector)
{
	return ((vector->x * vector->x) + (vector->y * vector->y));
}

Member functions are effectively just free functions with an extra implicit parameter.
In other words, they have no extra cost compared to using non-member functions.

All that said, there is precisely one case where a class will consume more memory than just the memory it takes to store its member variables, and that’s when it has virtual functions (whether or not those functions are inherited from a parent class).
As a result of having virtual functions, the class must then have a hidden implicit member variable - a pointer that points to its virtual function table (or ‘vtable’).
That field is the size of a pointer (sizeof(void *)).
The RAM cost of that extra field is negligable in comparison to the cost of the virtual function table.

In other words: use classes to your hearts content - member variables and member functions have no extra cost.
Just be sure to avoid virtual functions.


Based on my college experience (*cough* Visual Basic *cough*) I’m betting this is true.

I’d be guessing though, I’ve never been on a computer science course.