Maze Runner [V1]

(Scott) #21

If you’re concerned, you could calculate a checksum and save and check it against the data, for a bit more security (along with also setting a “signature” if desired).

1 Like
(Scott) #22

Correct. Originally, Arduino only provided EEPROM.write, which always writes to EEPROM, even when unnecessary because the value to be written is what was already last saved at that location. EEPROM.update was added later to avoid that situation, thus sometimes speeding things up and saving wear on the EEPROM if you blindly use values to write.

Note that EEPROM.put is also “intelligent”, like EEPROM.update.

I’m sorry but I’m kind of busy, so haven’t spent time analysing some of your more complicated expressions. I’ve just been reporting more obvious things that I’ve seen.

:+1:

Help, I'm struggling with EEPROM!
#23

That’s fine, its irrelevant if update can tell not to write the same data twice I don’t need to handle that manually.

1 Like
(Pharap) #24

This is exactly what I did in Minesweeper.

There’s other benefits to using a checksum as well though.
One of the reasons I used a checksum is because it can be used to detect when another game has overwritten your game’s save data.

My implementation is probably a bit overkill for this game because of it’s backwards and forwards compatibility features (and I still need to fix the undefined behaviour) but the checksum approach solves all the problems of the ‘2 byte id’ approach.

Should we ignore the syntax errors?

Sylistically mixing unsigned char and uint8_t is a bad idea.
On the Arduboy they are implemented the same but they actually have different semantics.

uint8_t means “this absolutely must be 8 bits”.
unsigned char means “the smallest addressable unit of memory”.

If in doubt, pretend unsigned char is actually 9 bits (or 10 bits, or 16 bits etc.) rather than 8 bits,
and then ask yourself if the code is still correct.

Also look at what the function actually returns (EEPROM.read returns a uint8_t).

(Also using macro case for anything other than macros is probably a bad idea.)

Yes, but possibly not like that.
Assuming you have an array of uint8_t called array:

EEPROM.update(address + (index * sizeof(array[0])), array[index]);

(Technically the sizeof(array[0]) part isn’t strictly necessary because it should be 1 if your array is an array of uint8_t, but it’s good practise to ‘say what you mean’ and it will be optimised away anyway.)

Personally I try to avoid the EEPROM class and use the eeprom_update_x and eeprom_read_x functions because I’ve found it results in less progmem usage in some situations.

I even wrote my own handy eeprom functions to make it easier to use.
(E.g. it’s possible to do Eeprom::update(address, array) and have the whole array handled without needing sizeof.)

avr-libc has (as far as I’m aware) always provided eeprom_update_byte (et al).

As far as I’m aware EEPROM has never actually been needed,
it’s just a wrapper that introduces (in my opinion) too much indirection and clutter.

Not to say that a wrapper couldn’t be useful,
but the EEPROM library is a very flawed implementation.

1 Like
(Scott) #25

I suspect that Arduino provides the EEPROM class as an attempt to make EEPROM functions standard across various processor architectures. There no guarantee that the EEPROM functions in avr-libc will be available for other Arduino supported processors.

(Pharap) #26

Had the EEPROMClass class not existed,
all the other architectures would have had to do is implement the missing functions from avr-libc.

There’s only a handful of functions from avr-libc that are surplus to what an implementation of the C standard library would have.
(I say that from experience - I’ve reimplemented almost all of avr-libc for the Pokitto.)

#27

I realized I don’t load the stuff I’m saving. I’ve added:

 minSteps = EEPROM.read(EEPROM_arrayTwoSaveLocationSteps);

where minSteps is:
uint16_t minSteps[MAX_LEVEL] {0, 0, 0, 0, 0, 0,0,0};

And am receiving this error:

incompatible types in assignment of 'uint8_t {aka unsigned char}' to 'uint16_t [8] {aka unsigned int [8]}'

I’m guessing this has something to do with pointer decay? What’s the “proper” way to save/ load an array? Do I need to iterate through the array and save each member individually using the pointer size to increment the location in EEPROM?

(Pharap) #28

If this is a global variable then it will be zero-initialised, so a simple uint16_t minSteps[MAX_LEVEL]; will suffice.

(MAX_LEVEL ought to be a constexpr variable of type size_t named in a case other than macro case, e.g. maxLevel, max_level or MaxLevel depending on your preferred style.)

Sort of.
It’s because you’re trying to write as if EEPROM.read is reading the whole array at once.
EEPROM.read only reads 1 byte at a time.

Hence why you get a type mismatch error - you’re trying to assign a value of uint8_t (1 byte) to an array.

(An array can never be directly assigned like that with any type, only its elements can be assigned to.)

There are several different ways.

If you with to use EEPROM.h then you must either do:

uint16_t eepromIndex = /*minSteps start*/;
for(uint8_t index = 0; index < maxLevel; ++index)
{
	// For uint8_t
	minSteps[index] = EEPROM.read(eepromIndex);
	++eepromIndex;
}

Or:

uint16_t eepromIndex = /*minSteps start*/;
for(uint8_t index = 0; index < maxLevel; ++index)
{
	// For uint16_t
	EEPROM.get(eepromIndex, minSteps[index]);
	++eepromIndex;
}

Depending on whether you want to read a uint8_t or a uint16_t.

Otherwise you can do one of the following…

For uint8_t:

const uint8_t * eepromPointer = reinterpret_cast<const uint8_t *>(/*minSteps start*/);
for(uint8_t index = 0; index < maxLevel; ++index)
{
	// For uint8_t
	minSteps[index] = eeprom_read_byte(eepromPointer);
	++eepromPointer;
}

For uint16_t:

const uint16_t * eepromPointer = reinterpret_cast<const uint16_t *>(/*minSteps start*/);
for(uint8_t index = 0; index < maxLevel; ++index)
{
	minSteps[index] = eeprom_read_word(eepromPointer);
	++eepromPointer;
}

For either:

const uint16_t * eepromPointer = reinterpret_cast<const uint16_t *>(/*minSteps start*/);
eeprom_read_block(&minSteps[0], eepromPointer, sizeof(minSteps));
1 Like
#29

How does the EEPROM index work? Like would

 uint16_t EEPROM_START = 1;

Be any different from

 uint8_t EEPROM_START = 1

I’m confused on how increasing the index by one int16_t works for both data thats 8bits and 16 bits long in the first two examples.

(Pharap) #30

1024 bytes from 0 to 1023, but the first 16 is used by Arduboy2 for settings.
If you’re not touching anything higher than 255 then you can get away with uint8_t, but generally you should use uint16_t.

(You should use EEPROM_STORAGE_SPACE_START rather than 16 though, just in case.)

Good point, the second example should be eepromIndex += sizeof(minSteps[0]).
This is why it’s bad to multitask.

uint16_t eepromIndex = /*minSteps start*/;
for(uint8_t index = 0; index < maxLevel; ++index)
{
	// For uint16_t
	EEPROM.get(eepromIndex, minSteps[index]);
	eepromIndex += sizeof(minSteps[0]);
}

(Pointers don’t have this problem of course, which is probably why the avr API prefers them.)

#31

Ok that makes more sense. I thought there might of been an error since your pointer examples where different sizes like I thought they should be.

Also, I was thinking of condensing the levels unlocked array into a single byte like in the third issue of the arduboy magazine so I could only use one byte per 8 levels. Would that be less perfereable though since it would be writing the same byte of eeprom 8x as much vs writing eight bytes once each time?

(Pharap) #32

I don’t think it would be too much of an issue.

If you’re worried about it though, you could save the first 8 levels in bit 0 of the first 8 bytes and then for the second 8 levels write to bit 1 of the same 8 bytes.

E.g.
Bit%20Alternation

That would do a bit of wear levelling and still allow condensed data.

#33

In the article they use byte as in byte bitArray {0B00000000}; uint8_t would work the same right? It appears it is.

Anyway, I think this should work but when I restart my arduboy it doesn’t appear to be saving. Here’s my current EEPROM.h file:

#pragma once
#include "GameEngine.h"
#include "Globals.h"
#include "Quads.h"
 uint16_t EEPROM_START_C1 = EEPROM_STORAGE_SPACE_START +373; 
 uint16_t EEPROM_START_C2 = EEPROM_START_C1 + 1;
 uint16_t EEPROM_arrayOneSaveLocationClears = EEPROM_START_C2 + 1;
 uint16_t EEPROM_arrayTwoSaveLocationSteps = EEPROM_arrayOneSaveLocationClears + 10;

void initEEPROM() {

  uint16_t c1 = EEPROM.read(EEPROM_START_C1);
  uint16_t c2 = EEPROM.read(EEPROM_START_C2);

  if(c1 != 1 || c2 != 0) 
  {    
      EEPROM.update(EEPROM_START_C1, 0);
      EEPROM.update(EEPROM_START_C2, 93);
      EEPROM.put(EEPROM_arrayOneSaveLocationClears, levelsCleared); //bool array of levels beaten
      EEPROM.put(EEPROM_arrayTwoSaveLocationSteps, minSteps);       //uint16_t array of number of steps taken  
  }
  else
  {
      const uint16_t * eepromPointer = reinterpret_cast<const uint16_t *>(EEPROM_arrayTwoSaveLocationSteps);
      for(uint8_t index = 0; index < MaxLevel; ++index)
      {
        minSteps[index] = eeprom_read_word(eepromPointer);
        ++eepromPointer;
      }
      const uint8_t * eepromPointer2 = reinterpret_cast<const uint8_t *> (EEPROM_arrayOneSaveLocationClears); //bitarray
      for(uint8_t index = 0; index < MaxLevel; ++index)
      {
        minSteps[index] = eeprom_read_byte(eepromPointer2);
        ++eepromPointer;
      }

  }
}

void saveEEPROMLevel(){
  const uint8_t * eepromPointer = reinterpret_cast<const uint8_t *> (EEPROM_arrayOneSaveLocationClears);
      for(uint8_t index = 0; index < MaxLevel; ++index)
      {
        EEPROM.update(eepromPointer, levelsCleared[index]);
        ++eepromPointer;
      }
  
}

void saveEEPROMSteps(){
  const uint16_t * eepromPointer = reinterpret_cast<const uint16_t *>(EEPROM_arrayTwoSaveLocationSteps);
      for(uint8_t index = 0; index < MaxLevel; ++index)
      {
        EEPROM.update(eepromPointer, minSteps[index]);
        ++eepromPointer;
      }
}

And the method where they’re called:

void GameEngine::highscoreUpdate(){
  if(minSteps[level] == 0){
    minSteps[level] = character.getSteps();
    saveEEPROMSteps();
  }
  else if(minSteps[level] > character.getSteps()){
    minSteps[level] = character.getSteps();
    saveEEPROMSteps();
  }

  if(bitRead(levelsCleared[(level+1)%8], (level+1)%8)==false){
    bitSet(levelsCleared[(level+1)%8], (level+1)%8);
    saveEEPROMLevel();
  }
}

I know that the arrays are being written, otherwise I wouldn’t see the highscores and the level select would be locked still

(Pharap) #34

The sad truth is this:

So yes, exactly the same.
(But uint8_t is obviously more portable. Especially in C++17.)

C++11 doesn’t have binary literals…

Once again, the sad truth is…

These can be uint8_t. read only reads 1 byte worth of data.

Problem #1: 0 is not 1 and 93 is not 0.

If levelsCleared is an array then I’m not sure this will work.
I suspect the array decays to a pointer before being bound to T.

That second line should be ++eepromPointer2;

Might I suggest some explicit block scoping?

void initEEPROM()
{
	uint8_t c1 = EEPROM.read(EEPROM_START_C1);
	uint8_t c2 = EEPROM.read(EEPROM_START_C2);

	if(c1 != 1 || c2 != 0) 
	{    
		EEPROM.update(EEPROM_START_C1, 0);
		EEPROM.update(EEPROM_START_C2, 93);
		EEPROM.put(EEPROM_arrayOneSaveLocationClears, levelsCleared); //bool array of levels beaten
		EEPROM.put(EEPROM_arrayTwoSaveLocationSteps, minSteps);       //uint16_t array of number of steps taken  
	}
	else
	{
		{
			const uint16_t * eepromPointer = reinterpret_cast<const uint16_t *>(EEPROM_arrayTwoSaveLocationSteps);
			for(uint8_t index = 0; index < MaxLevel; ++index)
			{
				minSteps[index] = eeprom_read_word(eepromPointer);
				++eepromPointer;
			}
		}

		{
			const uint8_t * eepromPointer2 = reinterpret_cast<const uint8_t *> (EEPROM_arrayOneSaveLocationClears); //bitarray
			for(uint8_t index = 0; index < MaxLevel; ++index)
			{
				minSteps[index] = eeprom_read_byte(eepromPointer2);
				++eepromPointer2;
			}
		}
	}
}

This way eepromPointer is out of scope by the second block, so you can’t make that mistake.

(Although some people say this is confusing.)

The pointer approach doesn’t work with the methods in the EEPROM class.

eeprom_read_x, eeprom_write_x and eeprom_update_x use pointers.
The EEPROM class (technically the EEPROMClass class) uses int instead and does its own casting.

EEPROM.update only accepts uint8_t as a second argument.

#35

What does this imply exactly? Since I only have 8 levels and I went with the spreading out method you mentioned I only am ever checking the first bit in the first byte. I’m guessing if I checked the second bit it would be true also?
It’s defined as 0B00000001

Spent too much time at puter. Should of noticed that since I changed them on purpose thinking it was good to go.

I was thinking that might be the case but I only need to write one bit as true if it’s a "new save’ and it appeared to be working correctly. (Probably not though considering the first issue)

Also too much puter time.
I prob should of just done that instead of making a second variable knowing they would both go out of scope anyway.

So just use += sizeof(array[0])

Would put work then? Or do you need to do something different for data that’s larger than a byte? (Like read_word vs byte)

(Pharap) #36

That 0B... aren’t binary literals, they’re hideous macros pretending to be binary literals and thus aren’t portable (unless you also port binary.h).

They’ll work, but you should probably prefer to use hexadecimal.
(Or (1 << 0), (1 << 1) etc.)

I’d need to test to be sure. I’ll do that tomorrow.

Forcing block scope isn’t something I see a lot, but I’ve also never seen it called bad practice either,
so I think the jury might still be out.

I think the usual advice would be either pick better variable names to avoid clashes or split the code up into more functions, but enforced unnamed scope is a cheap solution that comes in handy sometimes.

And drop the pointers for uint16_t indices.
(That’s assuming you want to use the EEPROM class.)

It should do.

eeprom_read_word and eeprom_read_byte are different beasts from a different API.

I’ll leave these links here for you to investigate.

The original avr-libc API:

https://www.nongnu.org/avr-libc/user-manual/group__avr__eeprom.html

The EEPROM API provided by Arduino:

For what it’s worth, I think EEPROM.h is a bad example. I think it tries to be clever but ends up being a big mess and causing too much indirection in the process.

I mean honestly, on what planet is this considered ‘readable’ code?
(Presumably planet Chris Andrews or planet David Mellis…)

EERef &operator +=( uint8_t in ) { return *this = **this + in; }
EERef &update( uint8_t in ) { return in != *this ? *this = in : *this; }

Note that EEPROMClass indirectly depends on eeprom_read_byte and eeprom_write_byte from avr-libc:

uint8_t operator*() const { return eeprom_read_byte( (uint8_t*) index ); }
EERef &operator=( uint8_t in ) { return eeprom_write_byte( (uint8_t*) index, in ), *this; }

(And yes, that second one is the comma operator in actual use in an actual code base in a non-constexpr context. I despair.)


That’s my last reply for the night, I was supposed to leave ages ago, it’s really late.

1 Like
#37

I’m not entirely sure now what the issue is. It was loading, however I had it loading the steps score over the levels cleared bit array, after fixing that It no longer appears to load anything. I’m guessing I still have an error in the saving functions. I’m at a bit of a loss though at this point.

I’ve branched for the EEPROM stuff here:

Not that I need to change the nomeclation of levels cleared to levels unlocked with how I’m currently using that array.

(Pharap) #38

I’ve had a quick look, I’ll look more when I get back later.

For now you might be best off just stuffing everything you want to save into a single struct and using EEPROM.put and EEPROM.get on that.

When I looked I think I spotted a bug here:

Also I just wrote this quickly, you might want a look:

Nomenclature?

(Simon) #39

Declaration?       

#40

Its this one lol. Originally it was a flag to say you beat the level but the way I’m using it is that the levels unlocked, not cleared.

The arrays size is 2 bytes * 8 incidences, 16 bytes. But if I add anymore levels it’ll increase by 2bytes per level. Neither seem to be working though, steps or levels cleared. (Both function in game correctly, its just when you restart the arduboy, so its Either not saving, not loading, or resetting the EEPROM.

Wouldn’t a struct containing an array have the same issue as a saving an array? I don’t really know how that would be different memory wise/layout.

My only concern is that the way Its organised right now I can freely add levels without breaking saves up to 64 total levels (not that I anticipated making another 50 levels). Every new level requires a new index in the steps array however. So keeping that compatibility is a goal

Also there’s another issue with % that should be a / in there too, but that wont’t matter until I add more levels, and It’s fixed, just not pushed yet if you notice that. in the highscoreUpdate() method.