Questions about a menu in an SD project


(Jon Raymond) #21

I’ve made some good headway on this over the last week.

Below is the current code

/* 
  App Loader
  Author: Jon Raymond
  Date: Febuary 20th 2019
  Version: 0.1

  This program is to be used in conjunction with the bootloader developed by Myndale (https://github.com/Myndale)
  which can be found here (http://legacy.gamebuino.com/wiki/index.php?title=Bootloader)
  App Loader reads hex file names (SFN) from the root drive of the SD card, sorts them and presents user with a selection menu
  
  ------------------------------------------------------------
  "THE BEERWARE LICENSE":
  Jon Raymond pieced together this abomination of code. As long as you retain this 
  notice, you can do whatever you want with this stuff. If we
  meet someday, and anything here helped you, you can
  buy me a beer in return. Cheers
  ------------------------------------------------------------  
  
  Program based off work from the below sources 
   
  Sort an array of Characters
  http://www.goodliffe.org.uk/arduino/sort_array.php
  Author: Paul Goodliffe
  Date: 2014

  Programming insight from Pharap
  https://github.com/Pharap

*/

#include <U8x8lib.h> //https://github.com/olikraus/U8g2_Arduino/archive/master.zip
  U8X8_SH1106_128X64_NONAME_HW_I2C u8x8(/* reset=*/ U8X8_PIN_NONE);
//  U8X8_SSD1306_128X64_NONAME_4W_HW_SPI u8x8(/* cs=*/ 12, /* dc=*/ 4, /* reset=*/ 6);  //  Arduboy 10 (Production, Kickstarter Edition)
  
#include <SD.h>
#include <SPI.h>

#define load_game (*((void(*)(const char* filename))(0x7ffc/2)))

#define MaxInArray 50
char *fileName[MaxInArray]; // Pointers to character strings, address to be determined by malloc.
byte arrayCount = 0;
byte menuStart = 0;
int8_t menuChoice = 0;
const byte menuRows = 8;
byte buttonEvent = 0;
char loadName[13];

void setup()
{
//  Serial.begin(9600);
  
   u8x8.begin(/*Select=*/ 4, /*Right/Next=*/ U8X8_PIN_NONE, /*Left/Prev=*/ U8X8_PIN_NONE, /*Up=*/ 9, /*Down=*/ 6, /*Home/Cancel=*/ U8X8_PIN_NONE); // Gamebuino
//   u8x8.begin(/*Select=*/ 7, /*Right/Next=*/ A1, /*Left/Prev=*/ A2, /*Up=*/ A0, /*Down=*/ A3, /*Home/Cancel=*/ 8); // Arduboy 10 (Production)

   u8x8.setFont(u8x8_font_5x7_r);   
   //u8x8.setFont(u8x8_font_chroma48medium8_u);
   //u8x8.setFont(u8x8_font_amstrad_cpc_extended_u);
   //u8x8.setFont(u8x8_font_pxplustandynewtv_u);
   //u8x8.setFont(u8x8_font_pxplusibmcgathin_r);
   //u8x8.setFont(u8x8_font_pcsenior_r);
   //u8x8.setFont(u8x8_font_pressstart2p_r);

   u8x8.setCursor (3,2);
   u8x8.println(F("App Loader"));
   u8x8.setCursor (2,3);
   u8x8.print(F("Version 0.1"));
   delay (700); // Delay so splash screen is visible
   
   populateFileArray();
//  printArray();

   sortFileArray();  
//  printArray();

    drawMenu();

}

void loop()
{
    checkButtons();
}

void checkButtons()
{
    if ( buttonEvent == 0 )
        buttonEvent = u8x8.getMenuEvent();
        
    if (buttonEvent == U8X8_MSG_GPIO_MENU_SELECT) {
        //Do the dirty things here... Needs rework
        strcpy(loadName, fileName[menuChoice]);
        strcpy(strstr(strupr(loadName), ".HEX"), "");
                  u8x8.clear();
                  u8x8.println(F("LOADING..."));
                  u8x8.print(loadName);
                  load_game(loadName);
      buttonEvent = 0;   // Not needed as function flashes new Hex
    }
    if (buttonEvent == U8X8_MSG_GPIO_MENU_UP) {
      
      menuChoice--;
      if (menuChoice < 0){ // Loop menu top to bottom
        menuChoice = (arrayCount - 1);
        menuStart = (arrayCount - menuRows);
        }
      if (menuChoice < menuStart){ // Scroll items down
        menuStart--;
        }
      buttonEvent = 0; // Clear button event
      drawMenu();
     
    }
    if (buttonEvent == U8X8_MSG_GPIO_MENU_DOWN) {
      menuChoice++;
      if (menuChoice >= arrayCount) { // Loop menu bottom to top
        menuChoice = 0;
        menuStart = 0;
        }
        if ((menuChoice - menuStart) >= menuRows){ // Scroll items up
          menuStart++;                
          }
      buttonEvent = 0; // Clear button event
      drawMenu();     
    }
}

void drawMenu()
{
    u8x8.home(); // Set Cursor (0,0)   
    for(uint8_t i = menuStart; i < (menuStart + menuRows); ++i) {     
      u8x8.setCursor(0,(i - menuStart));
      u8x8.print(i+1);
      if (i < 9) u8x8.print(' '); // Locates cursor and clears double digit numbers
      u8x8.print((i == menuChoice) ? '>' : ' '); // Display the selector character
      u8x8.print(fileName[i]);
        for(byte s = (strlen(fileName[i])) ; s < 13; ++s ) // Hack to clear long file names from display without full clear
          u8x8.print(' '); 
    }
}

void populateFileArray()
{
//  freeMessageMemory();  // Start by freeing previously allocated malloc pointers
    
    while (!SD.begin(10)) {
      u8x8.clear();
      u8x8.setCursor(3,3);
      u8x8.println(F("Insert SD"));
     }
    File root = SD.open("/");

    while(true) {    
     File entry =  root.openNextFile();
     if (!entry) {
       break;   // no more files
       }       
      // Filter for only files we want
      if ( arrayCount < MaxInArray && ( ( strcasestr(entry.name(), ".HEX") != NULL ) ) && !strstr(entry.name(),"LOADER.HEX") )
        {
        fileName[arrayCount] = (char *)malloc(13);
        sprintf(fileName[arrayCount],"%s",entry.name()); 
        arrayCount++;
        }
      entry.close();
     }
}
//void printArray()
//{
//  Serial.println(F("The array currently holds :"));
//  for (i = 0; i< arrayCount; i++)
//    {
//      Serial.println(fileName[i]);
//    }
//    Serial.println();
//}

/* -----------------------------------------------------------------------------------------------
 switchArray - This function takes the element in the array that we are dealing with, and 
 switches the pointers between this one and the previous one.
  ----------------------------------------------------------------------------------------------- */
void switchArray(byte value)
{
 // switch pointers i and i-1, using a temp pointer. 
 char *tempPointer;

 tempPointer = fileName[value-1];
 fileName[value-1] = fileName[value];
 fileName[value] = tempPointer;
 }

/* -----------------------------------------------------------------------------------------------
 This is a real neat function. It decides whether the first string is "less than"
 or "greater than" the previous string in the array. Input 2 pointers to chars;
 if the function returns 1 then you should switch the pointers.
  ----------------------------------------------------------------------------------------------- */

// Check 2 character arrays; return FALSE if #2 > 1; 
// return TRUE if #2 > #1 for the switch. Return 1 = TRUE, 0 = FALSE

byte arrayLessThan(char *ptr_1, char *ptr_2)
{
  char check1;
  char check2;
  
  int i = 0;
  while (i < strlen(ptr_1))    // For each character in string 1, starting with the first:
    {
        check1 = (char)ptr_1[i];  // get the same char from string 1 and string 2
        
        //Serial.print("Check 1 is "); Serial.print(check1);
          
        if (strlen(ptr_2) < i)    // If string 2 is shorter, then switch them
            {
              return 1;
            }
        else
            {
              check2 = (char)ptr_2[i];
           //   Serial.print("Check 2 is "); Serial.println(check2);

              if (check2 > check1)
                {
                  return 1;       // String 2 is greater; so switch them
                }
              if (check2 < check1)
                {
                  return 0;       // String 2 is LESS; so DONT switch them
                }
               // OTHERWISE they're equal so far; check the next char !!
             i++; 
            }
    }
  return 0;  
}

/* -----------------------------------------------------------------------------------------------
 This is the guts of the sort function. It's also neat.
 It compares the current element with each previous element, and switches
 it to it's final place.
  ----------------------------------------------------------------------------------------------- */
void sortFileArray()
{
  
  int innerLoop ;
  int mainLoop ;

  for ( mainLoop = 1; mainLoop < arrayCount; mainLoop++)
    {
      innerLoop = mainLoop;
      while (innerLoop  >= 1)
          {
          if (arrayLessThan(fileName[innerLoop], fileName[innerLoop-1]) == 1)
            {
             // Serial.print("Switching ");
             // Serial.print(fileName[innerLoop]);
             // Serial.print(" and ");
             // Serial.println(fileName[innerLoop-1]);
              
              switchArray(innerLoop);
            }
            innerLoop--;
          }
    }
}

/* -----------------------------------------------------------------------------------------------
 You remember we have to free the malloc's ? Well, it's a simple function.
 The pointer points to nothing, and the memory that was used is 
  ----------------------------------------------------------------------------------------------- */
//void freeMessageMemory()
//{
//  // If we have previous messages, then free the memory
//      for (byte i=1; i<=arrayCount; i++)
//        {
//          free(fileName[i-1]);
//        }       
//    arrayCount = 0;
//}

Code isn’t anything special. It reads file names from the SD, sorts them and then the menu system displays the file names for the user to select and forward on to the bootloader. The menu can scroll file names up and down. Current code compiles to 16626 bytes of program storage and 1376 bytes of dynamic memory.

There are a couple areas that need improvement. Hopefully @Pharap can take a gander at them and weight in with his expertise.

When passing the file name to the boot loader it needs to have the “.HEX” removed. Full file name is read from the array and then the extension replaced with “”. I understand that strcpy is not the best way but I’m not sure how else to do it?

strcpy(loadName, fileName[menuChoice]);
strcpy(strstr(strupr(loadName), “.HEX”), “”);

Also, when the filename is read from the SD card into the array,

fileName[arrayCount] = (char *)malloc(13);
sprintf(fileName[arrayCount],"%s",entry.name());

Is using sprintf equally as bad as strcpy?

Ideally what I think I should be doing is truncating the file extension as it is read into the array. Would this not in theory reduce the array size by %30?. I’ve tried but I just don’t understand enough about malloc and shortening strings correctly to get it to work.

If anyone has input on these items or any other parts of the code I would love to hear them. I’ve learned a lot code wise with the adventure and still have a lot to learn so any advice or insight is very much appreciated.


(Pharap) #22

Weigh-in, what am I, Gabe Newell? (Episode 3 will be worth the weight wait.)

In my opinion, most of the str functions are pretty ugly.
Plus they all have too much undefined behaviour for my liking.
For example, strcpy:

The behavior is undefined if the dest array is not large enough.
The behavior is undefined if the strings overlap.

strupr isn’t even a standard function.

But in the case of an embedded system the other choices are limited.
The other option is to just manually write for loops to handle your conversions.
Although sometimes that actually works out cheaper so it can be worth the extra effort.
For now I’d say if the str functions work, stick with them for now and then try something else later.

I would suggest a few tweaks though.
Normally I’d suggest ditching strcpy and use strncpy because it’s safer.
However, I notice you’re converting the copied string to uppercase afterwards,
that means you’re iterating over the same string twice.
So instead I’d suggest fusing the two operations like so:

// Assuming you're using null terminated strings
// Copy and convert to uppercase at the same time
for(uint8_t index = 0; index < sizeof(loadName) - 1; ++index)
{
	char c = source[index];
	dest[index] = toupper(c);
	
	if(c == '\0')
		break;
}

Next, I’d suggest splitting the substring finding operation off onto its own line and making use of progmem.

const char searchString[] PROGMEM = ".HEX";
char * hexStart = strstr_P(&loadName[0], &searchString[0]);

Before you were using a strcpy to copy the null terminator off an empty string,
which seems a tad heavy-handed since you know it’s going to amount to just one char being copied.
Instead, you can just do this:

if(hexStart != nullptr)
{
	hexStart[0] = '\0';
}

It’s worse.

The logic is much more complicated because it has to handle a lot of special formatting characters,
so it eats up more time and more memory than a simple copy.

I’d suggest replacing with either strncpy or a manually written for loop.

E.g.

// - 1 ensures the string is null-terminated,
// even if the whole name isn't copied
strncpy(fileName[arrayCount], entry.name(), fileNameBufferSize - 1);

Or

for(uint8_t index = 0; index < fileNameBufferSize - 1; ++index)
{
	dest[index] = source[index];
	
	if(source[index] == '\0')
		break;
}

Yes, but it’s relatively complicated.
You could in fact fuse the uppercasing in as well.

I had an attempt at it, but be warned this isn’t tested in any way at all:


#include <ctype.h>

bool isHexHelper(const char * source)
{
	const char pattern[] PROGMEM = "HEX";
	for(size_t index = 0; index < 3; ++index)
	{
		char patternChar = pgm_read_byte(&pattern[index]);
		char testChar = toupper(source[index]);
		
		if(patternChar != testChar)
			return false;
	}
	return true;
}

void processFileName(char * dest, const char * source, size_t count)
{
	for(size_t index = 0; index < count; ++index)
	{
		char c = toupper(source[index]);
		dest[index] = c;
		
		if(c == '\0')
			return;
		
		if(c == '.')
		{			
			bool success = isHexHelper(&source[index + 1]);
			
			if(success)
			{
				dest[index] = '\0';
				return;
			}
		}
	}
}

Then you could do something like:

fileName[index] = static_cast<char *>(malloc(fileNameBufferSize));
processFileName(&fileName[index], entry.name(), fileNameBufferSize - 1);

Really it would be best to get rid of malloc and use a statically allocated array because malloc can fail and you have to remember to clear up after malloc (using free).

Though one advantage of malloc is that if you discover you’ve allocated more memory than you need, you can shrink the string.

Buf if you’re going to go with that approach then using new[] and delete[] would be better - no casting to worry about.


A small tip when dealing with menus.
Instead of correcting an index variable after-the-fact,
it’s cheaper (both in terms of speed and memory) to pre-empt it becoming invalid.

I.e. instead of:

--menuChoice;
// Loop menu top to bottom
if (menuChoice < 0)
{ 
	menuChoice = (arrayCount - 1);
	menuStart = (arrayCount - menuRows);
}

// Scroll items down
if (menuChoice < menuStart)
{ 
	--menuStart;
}

Try:

// Loop menu top to bottom
if (menuChoice > 0)
{
	--menuChoice;
}
else
{ 
	menuChoice = (arrayCount - 1);
	menuStart = (arrayCount - menuRows);
}

// Scroll items down
if (menuChoice < menuStart)
{ 
	--menuStart;
}

On arrayLessThan you talk about returning FALSE and TRUE,
but you’re not using the bool type.

You should return a bool and use the constants true and false instead of using byte (which is actually uint8_t, which is actually unsigned char when CHAR_BIT is 8) with the values 1 and 0.

Secondly, I’d like to point out:

while (i < strlen(ptr_1)) 

This calls strlen every single iteration of the loop,
which means at the end of each iteration you loop through the string.

strlen works by actually looping through the entire string to find the null terminator,
meaning it’s a very slow operation.
You need to be storing the result if you’re going to be using it a lot.


That said, since you’re using str functions everywhere,
did you know there’s a strcmp and strncmp?

They compare strings, returning:

  • a negative value if the first string is ‘less’ than the second
  • a positive value if the first string is ‘more’ than the second
  • zero if both are equal

The strings are compared ‘lexographically’.
I couldn’t find a definition of what that means exactly,
but I’m assuming it’s at least alphabetical,
which isn’t far from what you’re currently doing.

Your sort function is probably slow, but that’s ok,
sort functions are arguably one of the hardest things in programming.
There’s tons of sort functions and most of them are complicated to write and usually hard to understand.

I rewrote it to use strcmp and made a few other changes:

void sortFileArray()
{
	for(size_t mainLoop = 1; mainLoop < arrayCount; ++mainLoop)
	{
		for(size_t innerLoop = mainLoop; innerLoop >= 1; --innerLoop)
		{
			int result = strcmp(fileName[innerLoop], fileName[innerLoop-1]);
			if (result < 0)
			{
				// swap
				const char * temporary = fileName[innerLoop];
				fileName[innerLoop] = fileName[innerLoop - 1];
				fileName[innerLoop - 1] = temporary;
			}
		}
	}
}

Technically size_t should be used for indexing arrays because its defined as:

size_t can store the maximum size of a theoretically possible object of any type (including array).

I.e. on some platforms int isn’t big enough.

However, on an Arduino system, usually using uint8_t is cheaper, so sometimes it’s ok to use that.
The important thing is to use an unsigned type,
otherwise you’re wasting half the potential values (i.e. the negative numbers).

I also brought the variables closer to where they’re used.
Keeping variables as close as possible to the part of the code where they’re actually needed is good for both the compiler and the humans reading the code.

When you keep a local variable’s lifespan/scope as small as it needs to be, the compiler can optimise the code better because it knows when the variable is no longer used and thus can reuse the resources (i.e. registers, or RAM) that variable would otherwise be using.

As for humans, humans have limited memory capacity,
so the less variables they have to keep track of,
the more mental capacity they have free to focus on other parts of the code.

And lastly I swapped to using prefix ++.
For small types and built in types it makes no difference whatsoever,
but it’s a good habit to have because some C++ code use these things called ‘iterators’,
and using postfix ++ on them can sometimes cause an unneccessary object to be created.

Postfix ++ is actually equivalent to:

auto temp = object;
++object;
return temp;

Sometimes the compiler knows it can get rid of lines 1 and 3,
but it can usually only figure that out with simple types.
Some types it actually can’t do that optimisation for because of language rules that prevent it from doing so.


Hopefully that’s enough of an info dump for you.


(Simon) #23

I am pretty sure the compiler optimises this if it can determine that it will not affect other parts of the code. I have tried both pre- and post- and they compile to the same size.

Besides, who has ever heard of ++C?


#24

I always use prefix because it makes more sense to me then evaluate then increment. I think it’s the only style thing I’ve concluded on my own that aligns with @pharap


(Pharap) #25

What types did you test with?

Like I said, for fundamental types it typically makes no difference because the compiler will realise the temporary is unused and optimise it out,
but there are times when it legally can’t by the rules of the language.
In particular, when the = operator has side effects.

Say for example the = operator prints to std::cout,
the compiler legally wouldn’t be allowed to get rid of the = because of the side effects.

Obviously no = is going to print to std::cout (unless it’s a class written specifically to track when certain operations are being performed for debugging purposes),
but some iterators for container classes will have complex = operators.

If someone asked Bjarne, I’d bet he’d say he regrets not calling it ++C. :P

Technically speaking C++ doesn’t guarantee when the increment happens for postfix increment.
Which means that certain expressions involving ++, such as:

i = i++ + 1;

Are ‘undefined behaviour’ and thus could make demons fly out of your nose.

(Both prefix and postfix can be undefined behaviour depending on the usage.)

That’s why, with the exception of the increment in for loops,
I always write my increments/decrements on a separate line.

Interestingly, as of C++11, i = i++ + 1; is undefined behaviour, but i = ++i + 1; defined.
Similarly i = v[i++]; is undefined behaviour but i = v[++i]; is actually defined.
Another great reason to prefer preincrement.


One reason why I prefer ++i that has absolutely no impact on the code is that it’s more natural to say ‘increment i’ than ’i increment’.

Also ++i is a better expression of intent, and expressing one’s intent is paramount.


(Jon Raymond) #26

Wow thank you! I’m going to read and try to digest this great info. It will take me more than a couple reads. In the meantime here is a quick video of the App Loader V0.1 filmed with a potato. Apologies for the camera frame rate causing the screen to flicker.

There are a couple more pictures over on Twitter.


(Pharap) #27

No problem.
Let me know if you have any more questions or need something clarified. (Except butter.)

You get points for including a ruler in one of the pictures,
but then you lose those points because the ruler doesn’t use centimetres.


(Jon Raymond) #28

Done.

I looked further into this and it turns out the bootloader only accepts uppercase filenames so I’ve ditched converting them to uppercase as it’s not needed. I may in the future try to filter filenames with lowercase characters or look for a library that can convert file names to uppercase. Right now that’s future Jon’s problems.

I got this to work but couldn’t quite wrap my head around…

Possible to get a little further explanation? I guess I don’t understand it enough to get it to work.

Thank you for this. Worked great!

Truth be told I didn’t write any of sort code. It was all taken from http://www.goodliffe.org.uk/arduino/sort_array.php
It worked from the start and so I didn’t mess with it. I’ve since taken your advice and converted arrayLessThan to bool and added your rewritten sortFileArray function. Thank you, it worked perfectly.

I gave this a shot but got an compile error.

AppLoader_v0.2:176:86: error: cannot convert ‘char**’ to ‘char*’ for argument ‘1’ to ‘void processFileName(char*, const char*, size_t)’
processFileName(&fileName[arrayCount], entry.name(), fileNameBufferSize - 1);

Not sure where ‘char**’ is happening?

Aside from this I took a stab at a function that would

  • sequentially read out the full filename from the sorted array
  • search the SD card for that file
  • get the file size and write it to an array
  • shorten the filename
  • read and store the filename length in an array
  • write the shortened filename back to the original array.

As pretty much expected it was fraught with issues. The SD library was giving me grief, storing each file name length (byte) used more ram than my hack of reading the file name and adding spaces. I gave up and just shorten the filename when reading it into the first array. It seem to work decently and if I want the file sizes then that can also be future Jon’s problem.

I worked on the UI a bit. The shorter files are nicer to read and I used a better arrow. A short video (better potato quality) is below.

I had a go at a better boot screen. The demo with a standard array for the logo “tiles” works great but I don’t have the luxury of that extra ram in the Loader Program. I moved the logo “tiles” to PROGMEM for the boot screen but I can’t seem to read them back out correctly. The display library needs this

*void u8x8_DrawTile(u8x8_t *u8x8, uint8_t x, uint8_t y, uint8_t cnt, uint8_t tile_ptr); (ref)

Specifically - * tile_ptr : A pointer to the first tile. Total memory area are cnt*8 bytes.

I know I need something to point the tile_ptr to the PROGMEM but I have so far failed to figure out how. Any help is much appreciated!

Full code it attached below.

/* 
  App Loader
  Author: Jon Raymond
  Date: March 4th 2019
  Version: 0.2

  This program is to be used in conjunction with the bootloader developed by Myndale (https://github.com/Myndale)
  which can be found here (http://legacy.gamebuino.com/wiki/index.php?title=Bootloader)
  App Loader reads hex file names (SFN) from the root drive of the SD card, sorts them and presents user with a selection menu
  
  ------------------------------------------------------------
  "THE BEERWARE LICENSE":
  Jon Raymond pieced together this abomination of code. As long as you retain this 
  notice, you can do whatever you want with this stuff. If we
  meet someday, and anything here helped you, you can
  buy me a beer in return. Cheers
  ------------------------------------------------------------  
  
  Program based off work from the below sources 
   
  Sort an array of Characters
  http://www.goodliffe.org.uk/arduino/sort_array.php
  Author: Paul Goodliffe
  Date: 2014

  Programming insight from Pharap. 
  Would not be possible without his help
  https://github.com/Pharap

*/

#include <U8x8lib.h> //https://github.com/olikraus/U8g2_Arduino/archive/master.zip
  U8X8_SH1106_128X64_NONAME_HW_I2C u8x8(/* reset=*/ U8X8_PIN_NONE);
//  U8X8_SSD1306_128X64_NONAME_4W_HW_SPI u8x8(/* cs=*/ 12, /* dc=*/ 4, /* reset=*/ 6);  //  Arduboy 10 (Production, Kickstarter Edition)
//  U8X8_SH1106_128X64_WINSTAR_4W_HW_SPI u8x8(/* cs=*/ 15, /* dc=*/ 16, /* reset=*/ 14);  // same as the NONAME variant, but uses updated SH1106 init sequence
  
#include <SD.h>
#include <SPI.h>
//#include <ctype.h>

#define load_game (*((void(*)(const char* filename))(0x7ffc/2)))

#define MaxInArray 50
char *fileName[MaxInArray]; // Pointers to character strings, address to be determined by malloc.
byte arrayCount = 0;
byte menuStart = 0;
int8_t menuChoice = 0;
const byte menuRows = 8;
byte buttonEvent = 0;
char loadName[13];
bool indent = false;
//byte fileNameBufferSize = 8;
//byte fileSize[MaxInArray];
//byte fileNameSize[MaxInArray];
const char searchString[] PROGMEM = ".HEX"; 

  const byte tiles[] PROGMEM = { 0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X80,0XC0,0XE0,0X60,0X30,0X30,0X18,0X18,0X1C,0X1C,0X0C,0X0C,0X0C,0X0C,0X1C,0X1C,0X18,0X18,0X38,0X30,0X70,0XE0,0XC0,0X80,0X00,0X00,0X00};
  const byte tiles2[] PROGMEM = { 0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0XE0,0XF8,0X3E,0X0F,0X03,0X01,0X00,0X80,0X80,0X80,0X80,0X80,0X80,0X80,0X80,0X80,0X80,0XC0,0XC0,0X40,0X60,0X20,0X20,0X20,0X20,0X20,0XE1,0X23,0X27,0XFE,0XF8};
  const byte tiles3[] PROGMEM = { 0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X1F,0X3F,0XF2,0XC2,0X8E,0X3F,0XE3,0X81,0X81,0X80,0X80,0X80,0XB8,0XD8,0X61,0X1F,0X01,0X03,0X0E,0X18,0X36,0X2E,0X20,0XA0,0XB0,0XD8,0XEF,0X70,0X38,0X1F,0X07}; 
  const byte tiles4[] PROGMEM = { 0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X00,0X01,0X03,0X07,0X06,0X0E,0X0D,0X0D,0X1D,0X3D,0X78,0X70,0X30,0X3E,0X1E,0X18,0X18,0X1F,0X0F,0X0C,0X0C,0X07,0X07,0X07,0X00,0X00,0X00,0X00,0X00};

void setup()
{
//  Serial.begin(9600);
   u8x8.begin(/*Select=*/ 4, /*Right/Next=*/ 7, /*Left/Prev=*/ 8, /*Up=*/ 9, /*Down=*/ 6, /*Home/Cancel=*/ 2); // Gamebuino
//  u8x8.begin(/*Select=*/ 7, /*Right/Next=*/ A1, /*Left/Prev=*/ A2, /*Up=*/ A0, /*Down=*/ A3, /*Home/Cancel=*/ 8); // Arduboy 10 (Production)

   u8x8.setFont(u8x8_font_px437wyse700b_2x2_r);

 for( byte c = 0; c < 7; ++c ){
    
//    u8x8.drawTile((-4+c), 1, 5, tiles);
//    u8x8.drawTile((-4+c), 2, 5, tiles2);
//    u8x8.drawTile((-4+c), 3, 5, tiles3);
//    u8x8.drawTile((-4+c), 4, 5, tiles4);

    byte b = ((c/2));
    switch (b) {
  case 0:
    u8x8.setCursor (14,3);
    u8x8.print(F("A"));
    break;
  case 1:
    u8x8.setCursor (12,3);
    u8x8.print(F("Ap")); 
    break;
  case 2:
    u8x8.setCursor (10,3);
    u8x8.print(F("App"));
    break;
  default:
    u8x8.setCursor (8,3);
    u8x8.print(F("App "));
    u8x8.setCursor (2,5);
    u8x8.print(F("Loader"));
    break;
    }
    delay(30);

  }
    u8x8.setFont(u8x8_font_5x7_r);
    u8x8.setCursor (11,2);
    u8x8.print(F("v.2"));

  populateFileArray();
//  printArray();

  sortFileArray();  
//  printArray();

//  getFileInfo();
   
  delay (500); // Delay so splash screen is visible
  
  u8x8.clear();
  
  drawMenu();

}

void loop()
{
    checkButtons();
}

void checkButtons()
{
    if ( buttonEvent == 0 )
        buttonEvent = u8x8.getMenuEvent();
     
    // Button Select   
    if (buttonEvent == U8X8_MSG_GPIO_MENU_NEXT  || buttonEvent == U8X8_MSG_GPIO_MENU_SELECT) {
//    if (buttonEvent == U8X8_MSG_GPIO_MENU_SELECT) {
        indent = true;
        drawMenu();
        delay(100); //to see indent
        indent = false;
        drawMenu();
        delay(100); //to see indent
        strncpy(loadName, fileName[menuChoice],8); 

            u8x8.clear();
            u8x8.setCursor (3,2);
            u8x8.print(F("LOADING..."));
            u8x8.setCursor (5,4);
            u8x8.print(loadName);
            load_game(loadName);
            buttonEvent = 0;   // Not needed as fuction flashes new Hex
    }
    
    // Button Up
    if (buttonEvent == U8X8_MSG_GPIO_MENU_UP) {

     if (menuChoice > 0) {
        --menuChoice;
        }
     else { 
        menuChoice = (arrayCount - 1);
        menuStart = (arrayCount - menuRows);
        }
      if (menuChoice < menuStart){ // Scroll items down
        menuStart--;
        }
      buttonEvent = 0; // Clear button event
      drawMenu();  
    }

    // Button Down
    if (buttonEvent == U8X8_MSG_GPIO_MENU_DOWN) {
      menuChoice++;
      if (menuChoice >= arrayCount) { // Loop menu bottom to top
        menuChoice = 0;
        menuStart = 0;
        }
        if ((menuChoice - menuStart) >= menuRows){ // Scroll items up
          menuStart++;                
          }
      buttonEvent = 0; // Clear button event
      drawMenu();     
    }
    
}

void drawMenu()
{
    u8x8.home(); // Set Cursor (0,0)   
    for(uint8_t i = menuStart; i < (menuStart + menuRows); ++i) {     
      u8x8.setCursor(1,(i - menuStart));
      u8x8.setFont(u8x8_font_open_iconic_arrow_1x1);
      if (i == menuChoice && indent) u8x8.print(' ');
      u8x8.print((i == menuChoice) ? 'F' : ' '); // Display the selector character
      if (i == menuChoice && !indent) u8x8.print(' ');
      u8x8.setCursor(3,(i - menuStart));
      u8x8.setFont(u8x8_font_5x7_r); 
      u8x8.print(fileName[i]);
        for(byte s = (strlen(fileName[i])) ; s < 8; ++s ) // Hack to clear long file names from display without full clear
          u8x8.print(' '); 
 
       u8x8.setCursor(14,(i - menuStart));
      if (i < 9) u8x8.print(' '); // Locates cursor and clears double digit numbers
      u8x8.print(i+1);
    }
}

void populateFileArray()
{
//  freeMessageMemory();  // Start by freeing previously allocated malloc pointers
    
    while (!SD.begin()) {
      u8x8.clear();
      u8x8.setCursor(3,3);
      u8x8.println(F("Insert SD"));
     }
    File root = SD.open("/");

    while(true) {    
     File entry =  root.openNextFile();
     if (!entry) {
       break;   // no more files
       }       
      // Filter for only files we want
      if ( arrayCount < MaxInArray && ( ( strcasestr(entry.name(), ".HEX") != NULL ) ) && !strstr(entry.name(),"LOADER.HEX") )
        {       
//          fileName[arrayCount] = static_cast<char *>(malloc(fileNameBufferSize));
//          processFileName(&fileName[arrayCount], entry.name(), fileNameBufferSize - 1);
        strncpy(loadName, entry.name(),12);        
        char * hexStart = strstr_P(&loadName[0], &searchString[0]);       
        strncpy(strstr(loadName, hexStart),"",12); 
        fileName[arrayCount] = (char *)malloc(13);
        strncpy(fileName[arrayCount],loadName,12);
        arrayCount++;
        }
      entry.close();
     }
    root.close();
}


/* -----------------------------------------------------------------------------------------------
 This function decides whether the first string is "less than"
 or "greater than" the previous string in the array. Input 2 pointers to chars;
 if the function returns 1 then you should switch the pointers.
  ----------------------------------------------------------------------------------------------- */
bool arrayLessThan(char *ptr_1, char *ptr_2)
//byte arrayLessThan(char *ptr_1, char *ptr_2)
{
  char check1;
  char check2;
  
  byte i = 0;
  while (i < strlen(ptr_1))    // For each character in string 1, starting with the first:
    {
        check1 = (char)ptr_1[i];  // get the same char from string 1 and string 2
        if (strlen(ptr_2) < i)    // If string 2 is shorter, then switch them
            {
              return true;
            }
        else
            {
              check2 = (char)ptr_2[i];
              if (check2 > check1)
                {
                  return true;
                }
              if (check2 < check1)
                {
                  return false;
                }
             i++; 
            }
    }
  return false;
}

/* -----------------------------------------------------------------------------------------------
 This is the guts of the sort function. It compares the current element
with each previous element, and switches it to it's final place.
  ----------------------------------------------------------------------------------------------- */
void sortFileArray()
{
  
  int innerLoop ;
  int mainLoop ;

for(size_t mainLoop = 1; mainLoop < arrayCount; ++mainLoop)
  {
    for(size_t innerLoop = mainLoop; innerLoop >= 1; --innerLoop)
    {
      int result = strcmp(fileName[innerLoop], fileName[innerLoop-1]);
      if (result < 0)
      {
        // swap
        const char * temporary = fileName[innerLoop];
        fileName[innerLoop] = fileName[innerLoop - 1];
        fileName[innerLoop - 1] = temporary;
      }
    }
  }
}

//bool isHexHelper(const char * source)
//{
//  const char pattern[] PROGMEM = "HEX";
//  for(size_t index = 0; index < 3; ++index)
//  {
//    char patternChar = pgm_read_byte(&pattern[index]);
//    char testChar = toupper(source[index]);
//    
//    if(patternChar != testChar)
//      return false;
//  }
//  return true;
//}

//void processFileName(char * dest, const char * source, size_t count)
//{
//  for(size_t index = 0; index < count; ++index)
//  {
//    char c = toupper(source[index]);
//    dest[index] = c;
//    
//    if(c == '\0')
//      return;
//    
//    if(c == '.')
//    {     
//      bool success = isHexHelper(&source[index + 1]);
//      
//      if(success)
//      {
//        dest[index] = '\0';
//        return;
//      }
//    }
//  }
//}

//void printArray()
//{
//  Serial.println(F("The array currently holds :"));
//  for (i = 0; i< arrayCount; i++)
//    {
//      Serial.println(fileName[i]);
//    }
//    Serial.println();
//}


//void freeMessageMemory()
//{
//  // If we have previous messages, then free the memory
//      for (byte i=1; i<=arrayCount; i++)
//        {
//          free(fileName[i-1]);
//        }       
//    arrayCount = 0;
//}

void getFileInfo ()
  {

      for (byte f = 0; f < arrayCount; ++f) {
        strncpy(loadName, fileName[f],12);        
        char * hexStart = strstr_P(&loadName[0], &searchString[0]);       
        strncpy(strstr(loadName, hexStart),"",12); 
//        fileName[f] = (char *)malloc(13);
        strncpy(fileName[f],loadName,12);
//        fileNameSize[f]=(8-(strlen(loadName)));
//        Serial.print(loadName);
//        Serial.print('\t');
//        Serial.println(fileNameSize[f]);
        
    }
  }
//    while (!SD.begin()) {
//      u8x8.clear();
//      u8x8.setCursor(3,3);
//      u8x8.println(F("Insert SD"));
//     }
//    File root = SD.open("/");
//  {
//    for (byte f = 0; f < arrayCount; ++f) {
//     File entry = SD.open((fileName[f]));
//      Serial.print(entry.name());
//      Serial.print('\t');
//      Serial.println(entry.size(), DEC);
//      entry.close();
//    }
//  }  
//}

Being from a commonwealth country I can appreciate the use of centimetres. This being said, isn’t “Banana for scale” the international unit of measurement on the internet?


(Simon) #29

Oh … I thought the dime was universal measurement. :slight_smile:


(Pharap) #30

I wrote out my whole reply and then lost it by accident so now I have to write everything again…


So it’s the SD library that can’t handle lowercase characters?

So, what you’re looking at is this:

const char searchString[] PROGMEM = ".HEX";
char * hexStart = strstr_P(&loadName[0], &searchString[0]);

if(hexStart != nullptr)
{
	hexStart[0] = '\0';
}

The docs for strstr_P say:

The strstr_P() function returns a pointer to the beginning of the substring, or NULL if the substring is not found. If s2 points to a string of zero length, the function returns s1.

So when you get hexStart back, if it’s not nullptr it’s pointing to the . that’s part of the ".HEX" that’s been found.

If you then change that to a '\0', you null terminate the string at that point, meaning none of the functions that expect null-terminated strings will read the HEX part or anything that comes after it.

So basically you’re converting:

{ 'G', 'A', 'M', 'E', '.', 'H', 'E', 'X', '\0' }

To

{ 'G', 'A', 'M', 'E', '\0', 'H', 'E', 'X', '\0' }

(hexStart[0] = '\0'; is equivalent to *(hexStart + 0) = '\0';,
which is the same as *hexStart = '\0'; after optimisation.)

So essentially the allocated memory stays the same size but you’re stopping the functions that work on null-terminated strings from seeing the rest of the data.
This sort of thing is one of the reasons why null-terminated strings were chosen for use in C to begin with.
(Though since then length-prefixed strings have become more popular.)

That article and the code in it is full of issues and inaccuracies.

For example

char tempString[13]; //Allocate 50 bytes as a regular character array.

A char is 1 byte in size, so that would be 13 bytes, not 50

byte i;

Making the loop counter global:

  • Forces the compiler to allocate a byte of RAM for the variable
  • Forces the compiler to write back to that RAM variable before the function where it’s used exits

Whereas if the counter was local it could be kept in a register for its entire lifetime and never use any RAM whatsoever.

And of course the problems I mentioned with not using bool and using strlen as if it gave back a constant value.

There’s some other problems as well, but I’ll leave it there.
This person’s probably not a very experienced programmer so I don’t want to be too harsh with my criticisms,
but you get my point.

Sorry, that should be:

processFileName(fileName[arrayCount], entry.name(), fileNameBufferSize - 1);

I think I started writing &fileName[arrayCount][0] either out of habit or because I got confused because the array is called fileName rather than fileNames.

When I’m passing a pointer to an array I tend to write &array[0] rather than just array to emphasise that fact that an array is being converted to a pointer to the first element of the array.
In this case I would do fileName[arrayCount] rather than &fileName[arrayCount][0] because it’s an array of pointers rather than a 2D array.

That’s the C version. The C++ version doesn’t need a u8x8_t pointer.

Looking at the example I’m not sure it accepts progmem pointers.

The given example is:

uint8_t tiles[16] = { 0x0f,0x0f,0x0f,0x0f,0xf0,0xf0,0xf0,0xf0, 1, 3, 7, 15, 31, 63, 127, 255};
u8x8.drawTile(1, 3, 2, tiles);

If that was in progmem, it would look like this:

const uint8_t tiles[16] PROGMEM = { 0x0f,0x0f,0x0f,0x0f,0xf0,0xf0,0xf0,0xf0, 1, 3, 7, 15, 31, 63, 127, 255};
u8x8.drawTile(1, 3, 2, tiles);

You might have to ask the author about that one.
(@olikraus I think.)

Canada? (Assuming the French influence outweighs the American influence.)
Australia?

To be honest I’d be happy with inches even,
as long as it was an actual ruler in use and not a country-specific coin.

It’s upsetting the number of electronics websites that use American quarters for scale instead of rulers.

I’d honestly never heard of the meme before.
(Nor the ‘banana equivalent dose’ for that matter.)

Frankly a banana would be more useful,
at least I could nip out to the shops and buy a banana,
I can’t do that with a foreign currency.

(I could probably get a quarter from the bureau de change, but that would be much more expensive and they probably don’t deal in such low denominations.)


I’ve been meaning to ask, how are you using load_game?

#define load_game (*((void(*)(const char* filename))(0x7ffc/2)))

I’m assuming it’s casting a specific RAM address to a function pointer?
But it seems a bit odd.


(Jon Raymond) #31

I hate when that happens. Thanks for taking the time to rewrite it.

No, the SD enabled boot loader can only read uppercase file names. If you pass “Blink” to it, it will not be able to locate the file and will default to loading “LOADER” and if that is not there it will load whatever was stored in flash previously. The SD section of the boot loader I’m using is from the 2boots source. The boot loader is essentially a combination of circa 2014 Optiboot and 2boots. The source (direct download) for it is available but it’s well beyond my skill to update or mess with.

Correct. It causes a long-jump into the boot loader code causing it to load and launch from the SD card the name passed to it. The “0x7ffa/2” is a fixed constant representing the address of the function call in the boot loader memory. Interesting to also note that the boot loader allows the application to flash pages in application space which can be used for game patching or self-modifying code. A greater detailed explanation can be found at the first link I posted in this reply.

Thanks for the further explanation about strstr_P() function. I thought that’s how it worked in theory but it makes a lot more sense now.

It’s funny because I’m the same way when I see someone new to pcb design. I cringe at their layout. But on the other hand, if software works no matter how bad it is I’m like ¯\_(ツ)_/¯. Leave it be, I’m going to break it if I mess with it. You software folk are wizards in my eyes!

Okay, I wanted to make sure I wasn’t missing something really simple before asking.

Nailed it.

Apologies, seems I struck a nerve by accident. Here is a peace offering.


(Pharap) #32

In fairness it’s the first time it’s ever happened.

If I hadn’t rewritten it then I would have had to do it tomorrow anyway.

Odd.

I had a peek.
It’s written with some very ugly C code.
I could probably make sense of it if I had to,
but I don’t think I want to.

Anything that puts hexadecimal numbers on the right hand side of a >> is frown-worthy in my books.

I spotted it in the source code for the bootloader.
God knows why they used 0x7ffc / 2 instead of 0x3FFD.

This is why ‘magic numbers’ are evil - constants should always be named,
and particularly confusing constants should be explained with a comment.

I’m guessing it’s documented in the hardware documentation rather than the code.

At any rate, it can be written better:

using load_game_function = void (*)(const char *);
constexpr uintptr_t load_game_address = (0x7FFC / 2);
const load_game_function load_game = reinterpret_cast<load_game_function>(load_game_address);

You could do it without the type alias,
but I’d have to check how to make load_game const if you did.

Unfortunately the reinterpret_cast prevents constexpr from being used,
but I’m pretty sure the compiler will figure it out at compile time anyway.

If it is using more memory, try this instead:

void load_game(const char * filename)
{
	using load_game_function = void (*)(const char *);
	constexpr uintptr_t load_game_address = (0x7FFC / 2);
	
	const load_game_function load_game_internal = reinterpret_cast<load_game_function>(load_game_address);
	load_game_internal(filename);
}

Or at the very least:

using load_game_function = void (*)(const char *);
constexpr uintptr_t load_game_address = (0x7FFC / 2);
#define load_game (reinterpret_cast<load_game_function>(load_game_address))

If I had a penny for every time “have a look at the docs here” was the answer to a question,
I’d be able to buy myself an expensive coffee, or maybe a fancy hat. :P

I’d probably be one of those people.

I can understand and design circuits at the logic level
(I’ve drawn diagrams for things like binary full adders and barrel shifters before),
but my knowledge of things at the electrical level is almost nonexistant.

I can’t let it lie.
Even minor stylistic issues make my hair stand on end.
(I’m like the Adrian Monk of C++.)

In this particular case that sorting function was exceptionally bad for performance.
If strlen had been a constant-time operation then it wouldn’t have been too bad,
but strlen literally goes through the whole array and counts each character until it finds a '\0',
so every loop iteration was wasting time recounting the same information.

If you’d been handling more data or the chip had been slower then you’d have noticed a significant slowdown.

(Unless the compiler was noticing and caching the result of strlen, but I don’t think it’s allowed to do that.
Unless the function being from the standard library makes it an exceptional case.
So it’s possible I guess?)

I may seem like a wizard now, but once upon a time (less than a decade ago) I didn’t know a gigabyte from a megabyte,
or which part of a computer was the RAM and which was the CPU.

It’s as much about time and experience as it is about aptitude.

I had a look to make doubly sure, and it seems that the C++ API is just a wrapper for the C API and the C API version of drawTile calls a function pointer, so finding which function is actually doing the implementing to check if it’s reading RAM or progmem would be difficult.

Excellent. That was my first guess because it explains how you got hold of a quarter. :P

That more than makes up for it. :P

None of the rulers I own are that fancy.

I love the idea of a programmable device on a ruler, some of those look really handy.

Looks like your new gadget is ~65mm in width and ~30mm in height.
Much more useful than a quarter. :P


(Jon Raymond) #33

My understanding is that MS DOS is to blame for that. 8.3 Filenames

To be fair I think it was an alpha release and hasn’t been touched in 5 years.

I tried 0x3FFD. No bueno. Magic numbers are magic.

I tried all of your examples and all of them work. They seem to compile all to the same size.

That’s a fancy way of saying RTFM. :wink:

I’m currently experimenting with the SSD1306Ascii display library. Initial results show a 15% reduction in ram usage and 12% less flash compared the u8x8.


(Pharap) #34

I wasn’t aware that also restricted the character set to uppercase only,
I thought case insensitivity was just a Microsoft design decision to ‘dumb down’ computers for the general public.

That excuses some of the ugliness, but not some of the poor decisions, like using a global loop counter and treating strlen as if it were a constant time function.
(I was going to say O(1), but then I thought you might not know about big-O notation.)

Oops, my mistake, it’s 0x3FFE.
Either my finger slipped or I pressed the wrong button on the calculator.
This time I wrote a small C# program to verify it.

Ah, that’s the bit I was more concerned with.
That’s good to know.

In that case, pick the one you think seems most readable.

If you have any questions about any of them, feel free to ask.
E.g. If you aren’t sure about what reinterpret_cast means, I could explain it.

Yes, but hopefully more polite.

I don’t want to put people off asking me things, because sometimes the docs need to be ‘interpreted’ or ‘reworded’ a bit by someone who knows them better (i.e. who knows where to look and what the more fancy terminology means).

That’ll be the result of not using a screen buffer.
(Oddly I was just discussing screen buffers on one of the u8g2 library’s issues.)

If it’s fast enough then it makes sense to use that.


(Jon Raymond) #35

Yup that seems to work perfectly. Magic numbers debunked by math!

Well according to the U8x8 documentation… :grin:

U8x8

  • Text output only (character) device.
  • Only fonts allowed with fit into a 8x8 pixel grid.
  • Writes directly to the display. No buffer in the microcontroller required.

The lack of buffer was why I first initially choose it. My guess is that some of the overhead is sneaking in from its bigger brother U8g2.


(Kevin) #36


(Jon Raymond) #37

V.3 is coming along nicely.


(Kevin) #38

Oh wow, so this is with a bootloader for the 328p that supports flashing from SD?

How is this being done, do you have an application that reads through the SD card, and then pass a pointer back to the bootloader somehow to get it to flash the new game?


(Jon Raymond) #39

Correct.

Exactly. Only thing I would add is that the names are also sorted alphabetically for display in the menu.


(Kevin) #40

If it works on the 328p then I would imagine you could come very close to copy pasting the code from the bootloader into the one for the 32u4. You might need to change some registers but I can’t imagine too much is different. I don’t know about codespace and I don’t know how much it actually needs to transverse the fat system, but if all you are doing is pointing a memory address then I think reading out the data is mostly similar to what is being done on the EEPROM flash.

But I’m mostly an armchair quarterback on this stuff, don’t trust me.

I like your hardware though, keychain style, very cool!