First Application: Code Review Request

Help & Information
#1

Hi All,

I am a C# developer by nature and Arduino/C++ development is new to me. Lots of programming fundamentals seem to carry over like any language, but would like to become more polished.

Any tidbits or knowledge you might be able to transfer would be appreciated. :slight_smile: I am avidly reading the docs and community to improve myself, but it’s always fun talking with people and getting a code review.

Note: Still working on my development pipeline for making applications on Arduboy. It was mentioned I should have the folder be the same name as the .ino file. If I have a large application I would do this practice so uploading the code is easier with the Arduino IDE.

First Application Code - Updated URL
Bounces text like a screen saver around the screen
Up: Increases Speed
Down:Decreases Speed
A Button: Resets Speed
B Button: Pauses

It was so satisfying getting that stupid text to bounce around the screen.

PS: Anyone using PlatformIO in vs code? Super slick, but still working out making the Serial Monitor to work after uploading code. Seem to say the COM is busy.

Thanks,

– Lincoln

#2 
void setup() {
  randomSeed(analogRead(0));

The Arduboy2 library has the initRandomSeed() function to generate a random seed. Using it would make your code more portable to other architectures that use a port of the Arduboy2 library.

void setup() {
  arduboy.initRandomSeed();

If you want people who use the Arduino IDE to be able to compile and work with your code, you should have your main code in a .ino file instead of .cpp

#3

@MLXXXp I just updated the code to reflect your recommendation. Good point about the .cpp file to .ino. Forgot that was even a thing after setting up in PlatformIO. Thanks!

1 Like
#4

The pollButtons() function relies on a delay long enough to properly debounce a button between successive calls to it. The actual bounce time depends on the construction of the particular button but something greater than 50ms is a good rule of thumb. (Some really poor buttons can bounce for over 100ms.)

If your update speed becomes to fast, it could affect justPressed() reliably detecting a pushed button as a single press.

Most Arduboy sketches use nextFrame() in loop(), along with setFrameRate() or setFrameDuration() in setup(), to run at a constant frame rate. For your application, you could then use everyXFrames() to slow down the movement below the actual frame rate.

void loop() {
  if (!arduboy.nextFrame()) {
    return;
  }
  arduboy.pollButtons();

  if (arduboy.everyXFrames(speed) {
    // (do a move here)
  }
#5

You should name your .ino file as what you want your sketch to be known as. With the Arduino IDE, the name is what differentiates sketches, and the .ino file has to go in a folder of the same name. With main.ino your program will be known in Arduino as main.

#6

@MLXXXp Much better. A whole heck of a lot smoother now, but the way I had it made the text go faster. I noticed while reading the docs for this method it can be used to stagger things happening on the screen like the example of firing a shot. With my application it’s now staggering when the cursor gets updated, which is now x frames. I guess when it comes to game development humans can only react so quickly? So, putting a limit to speed balances playing and processing button clicks, huh?

#7

@MLXXXp oop, forget to push that change. lol its fixed now. :slight_smile:

#8

I may find more later, but off the top of my head:

* Disclaimer

Most of the time it won’t make a difference for Arduboy code because you’re unlikely to be dealing with complex iterators, but there are plenty of times in desktop code where using preincrement will avoid a copy that the compiler can’t legally optimise away because doing so would potentially remove side-effects and it’s (usually) not allowed to do that.

If you always prefer preincrement and predecrement over postincrement and postdecrement then you never have to worry about unelidable copies, and you have less undefined behaviour (see links 3 and 4) to worry about.

The closest thing there is to an ‘official’ record of good practice is the isocpp core guidelines:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines

This is more of a conditional warning:
If you have any intent to make your code portable, be cautious about using int - remember that it’s at least 16 bits, not exactly 16 bits and not exactly 32 bits. If you’re using int then you should make sure your code will work for both, otherwise use one of the fixed-width types (int16_t, int32_t, int_least16_t, int_least32_t et cetera) to ensure that you end up with a type of the exact size that you need.

Arduino specific:

  • If you have variables that never change you should set them when they’re declared rather than in setup()
  • Avoid String because it allocates on the free store (a.k.a. the heap)
    • There’s several different alternatives for getting the size of a string. More information can be provided upon request.
  • Prefer to use the smallest datatype possible. Usually that’s uint8_t or int8_t.

I believe bouncetext.ino should be called BounceText.ino (it might not make a difference on Windows but it probably makes a difference on Linux) and should be outside src, directly under BounceText.

I’ve never checked, but I believe it ought to be possible to have the .ino outside src be empty, which would meet the Arduino IDE’s requirement of having an .ino file and allow you to use a main.cpp file as your main code file.

Arduboy specific:

The typical pattern for an Arduboy loop() is:

void loop()
{
	if(!arduboy.nextFrame())
		return;
		
	arduboy.pollButtons();
	
	arduboy.clear();
	
	// update game state
	// draw game state
	
	arduboy.display();
}

Interfering with this pattern typically results in a non-constant framerate which may or may not cause issues later down the line.

#9

With that loop() structure, you can also usually eliminate the
arduboy.clear();
and use
arduboy.display(CLEAR_BUFFER);

If there is something left in the screen buffer at the end of setup(), such as the boot logo, parts of it may be superimposed on your first (and only first) frame but at typical frame rates that flaw will likely go by unnoticed by the user.

#10

If you do that though, you also have to remember to add arduboy.setCursor(0, 0); where arduboy.clear(); used to be.

#11

That’s true if you’re using text and assume the cursor is at 0, 0 at the start of each frame. This isn’t the case for a good number of sketches.

Something recommended in the Arduino style guide, that I like to see (which unfortunately is rarely done):

Put your setup() and your loop() at the beginning of the program. They help beginners to get an overview of the program, since all other functions are called from those two.

#12

:partying_face: My my my there is a lot to unpack here. lol Thanks for putting in the time to review this. I’ll need a moment to work through some of this. I have a new training project I am working so I think I will be trying to implement these concepts here: BeepBeep

In BeepBeep I am building a playground to testing tones, but needed a good class to easily move/update text around the screen. Will be showing a graphic of the tone frequency.

#13

Code hour with @MLXXXp and @Pharap

Without users like you we would for sure be lost in our quest to make awesome 8 - bit games.

1 Like
#14

Alight, I got some time to work through your suggestions and refactored my code to reflect this.

  • Updated enum to a class enum: I do like this better. The other way seemed kinda messy. Thought that was just how it went in c++.
  • I’ve read const should be used with non-static methods. Makes sense to me! constexpr should be used for always unchanging for compile.
  • WHY CAN’T I USE ALL CAPS. THEY ARE POWERFUL. I get it. Just saw it online somewhere and was like, sure. It seems naming type variables should be separated with underscores. IMO it looks messy. I like camelCase, but my C# is showing.
  • Can you explain why I can’t separate variables using the same types by commas? I did it on a whim and was supprised. Once I get comfortable with separating code to .h/.cpp files I doubt i’ll have so many long variable declarations.
  • static_cast vs (object) is interesting to research. Funny how much I take for granted in a higher level language like C#. Higher being it’s doing more my simple mind can’t comprehend.
  • ++i vs i++. Ehhhh, I kinda get it? For the (speed > 1) id want to use preincrement I don’t want speed to get less than one since that will stop my frames. Postincrement would increment past one?
  • regarding int_*, sure. int is much simpler with devices you don’t have to worry about space and ram, but this is what I LOVE about this type of development. It makes you worry about this. Brings ya back to early development circa 80s, not that I would know, but my father would be proud.
  • So far the only issue with all of these tips was adding display() at the end of the loop. This doesnt work for this specific application because I am updating the cursor every x frames. Otherwise it just flashes.

I decided to apply these to my bounce application since it was simple to apply. My BeepBeep application in the same repository will try to push this further.

Congrats, you made it through my train of thought post-refactor update to your recomendations. I’d make a badge for you if I could.

Edit:

Forgot to mention the whole folder to .ino thing. I am using PlatformIO on VS Code. When wanting to show code to someone I have some automation in place to just throw src into a folder of the project and call it good. I’d direct the person to that folder when referencing an application.

Edit 2:

.ino name convention: I’ve poured through a bunch of game repositories and haven’t seen upper camel case for .ino files a lot. Does it really matter?

Edit 3:

@Pharap Missed a lot in this post and forgot to ask you to go in dept of what I should use instead of the String object.

#15

I am curious about this code:

void loop() {
    if (!arduboy.nextFrame()) {
        return;
    }
    ...
    if (arduboy.everyXFrames(speed)) {
        if (!paused) {
            nextPosition();
            arduboy.setCursor(x_current, y_current);
            arduboy.print(title);
            arduboy.display(CLEAR_BUFFER);
        }
    }
}

I know it works but in a typical program, you would refresh the screen (arduboy.display(CLEAR_BUFFER)) every frame regardless of whether the text (or players in a game) have moved.

1 Like
#16

When displaying at the end of the loop it flickers. Not sure why but that’s why it’s there. A newby to this type of framework so I don’t have any reason besides it works. :cowboy_hat_face:

1 Like
#17

It was until C++11 - C++11 was a big overhaul that added a lot of useful features.
Many of C++'s flaws are stuff it’s inherited from C.

const applies to types, not functions, so it’s a bit more nuanced than that.
const should be thought of as meaning ‘read only’ and should be used with read-only data, but constexpr should be used instead of const when possible.

(Note: C++ literature doesn’t tend to use the word ‘method’, it prefers ‘function’. Particularly terms like ‘member function’ and ‘non-member function’/‘free function’.)

The C++ standard library uses lowercase and underscores,
but Arduino code tends to use Pascal case for types and camel case for functions/objects*,
so for Arduboy games you should probably stick to the Arduino style so your code is more consistent.

(* Note: In C++, anything that occupies memory is an object and not all objects have member functions, hence it’s “everything’s an object”, but in a different and more efficient way to Java’s idea of “everything’s an object”.)

You technically can, but it’s considered bad form.
The comma operator is rarely used and not widely understood so it generally harms readability.
(Also not all uses of a comma are instances of the comma operator which muddies the waters.)

A slightly less important but still valid reason is that the comma operator forces the left hand side of the expression to be sequenced before the right hand side which can interfere with the compiler’s ability to optimise by removing some of the flexibility it has to reorder operations.

I also learnt C# before C++ so I know what you mean.
The biggest leap is in dealing with pointers and having to be in charge of managing your own memory instead of relying on a GC.
Suddenly you actually have to understand how RAM works. :P

I’m not sure what you’re asking/saying.

It’s more the case that most of the more modern languages are targeting x86 computers so 32-bit and 64-bit have become the norm.
C++ was made back in the days where 16-bit computers were the norm and some CPUs uses word sizes that weren’t a multiple of 8.
(Although technically it inherited its int rules from C, which goes back further.)

As @filmote pointed out, most games just draw every single loop iteration.
In your case you’d need to move both the print and the display calls out of the if and into the loop so it’s always drawing.

There actually are badges on the forum, but only @bateske can make them.
(My favourite badge is empathetic.)

The platformio.ini is a big give away. :P

I also use VS Code (well, technically VS Codium, but same difference) but I use the Arduino plugin instead of PlatformIO because it behaves the same way the Arduino IDE would.

It’s not the casing itself that’s important, it’s that the name of the .ino file has to match the folder name.
Windows uses a case-insensitive file system, but Linux has a case-sensitive file system so it’s likely that the folder name and file name have to match exactly in regard to letter case.

I.e. if you want to make a file called bouncetext.ino then go ahead, but make sure it’s in a folder called bouncetext rather than BounceText (or vice-versa).

That’s going to take quite a bit of explaining and demonstration so I’ll come back to it, lest this comment grow any larger.

I looked at how you’ve updated your code on GitHub…

It won’t compile because this isn’t legal:

uint8_t 0;

After removing those two lines it all compiles.

Most likely you forgot to move setCursor as well.
I tried not moving that and it does indeed cause flickering.

Doing this works:

void loop() {
    if (!arduboy.nextFrame()) {
        return;
    }

    arduboy.pollButtons();

    if (arduboy.justPressed(UP_BUTTON)) {
        if (speed > 1) {
            --speed;
        }
    }
    if (arduboy.justPressed(DOWN_BUTTON)) {
        ++speed;
    }
    if (arduboy.justPressed(A_BUTTON)) {
        speed = 10;
    }
    if (arduboy.justPressed(B_BUTTON)) {
        paused = !paused;
    }
    if (arduboy.everyXFrames(speed)) {
        if (!paused) {
            nextPosition();
        }
    }

    arduboy.clear();
    arduboy.setCursor(x_current, y_current);
    arduboy.print(title);
    arduboy.display();
}

As does:

void loop() {
    if (!arduboy.nextFrame()) {
        return;
    }

    arduboy.pollButtons();

    if (arduboy.justPressed(UP_BUTTON)) {
        if (speed > 1) {
            --speed;
        }
    }
    if (arduboy.justPressed(DOWN_BUTTON)) {
        ++speed;
    }
    if (arduboy.justPressed(A_BUTTON)) {
        speed = 10;
    }
    if (arduboy.justPressed(B_BUTTON)) {
        paused = !paused;
    }
    if (arduboy.everyXFrames(speed)) {
        if (!paused) {
            nextPosition();
        }
    }

    arduboy.setCursor(x_current, y_current);
    arduboy.print(title);
    arduboy.display(CLEAR_BUFFER);
}
2 Likes
#18

If it were me, I would do:
x_min = y_min = 0;

1 Like
#19

I wouldn’t do that because I think it harms clarity/readability and I prefer to do only one assignment per statement.
For one thing the fact = is right associative instead of left associative which probably comes as a surprise to some people.

(Could be worse though, I’ve seen people do stuff like *(a = b) = c;.)

#20

Actually, I only do it when the objects I’m initialising are related to each other, in cases where (to me, at least) it enhances clarity/readability.