Switch doesn't give same results with different case ordering [Solved]

benoit · July 26, 2020, 11:36am

Hi everyone,

I bought my first Arduboy last week, and I have a lot of fun on playing and coding games ! Thanks for that amazing community, I have learned a lot from this forum.

So, I’m working on a program to “help” my 4 years old son to imagine adventures. It’s not really a game, but more some screens to go with its stories. For example, you have a radar screen that shows a scanner and some points are blinking and moving through the screen.

But I also have a screen which acts like a phone : you have a list of contacts (a monkey, a frog and a robot), you select one of them, an animation is displayed to represent the hanging call until the character picks up, and then you have the conversation.

My difficulty is that I handle this workflow with a switch case :

void display(Arduboy2 arduboy, ArduboyTones sound)
{
    switch (screen) {
        case 0:
            controlsContactsList(arduboy);
            contactList.displayConctactsListTitle(arduboy, selectedCharacter);
            contactList.displayContactsList(arduboy, selectedCharacter);
            break;
        case 2:
            displayConversation(arduboy);
            break;
        case 1:
            // Intro has not finished playing
            if (needToPlayHangingIntro) {
                needToPlayHangingIntro = hangingCall.introIsPlaying(arduboy, selectedCharacter);
                return;
            }

            // Hanging animation
            bool hanging = hangingCall.hanging(
                arduboy,
                sound,
                selectedCharacter
            );

            if (arduboy.justPressed(B_BUTTON)) {
                screen = 0;
            }

            if (hanging) {
                return;
            }

            // Animation finished, jump to last screen
            screen = 2;

            // Populates line buffer with random indexes of alphabet symbols
            for (int i=0; i<108; i++) {
                conversationLine[i] = random(0, 30);
            }
            animationTimer.updatePreviousTime();

            break;
    }
}

https://github.com/benoitjupille/exploration-tools/blob/9d85ecf168658370e03e0fb441b9635e65c5bc4d/tools/src/phone/Phone.h#L47

You can notice that i putted screen 2 before screen 1 in the switch cases order. In this order, everything works fine. But if I put the case 2 block after the case 1, then the screen 2 won’t play.

Could you help me by giving me clues on where to start looking please ? I’m not familiar with C++ language, so I was wondering if that’s a recurrent issue on this language, because I’ve never had this kind of problem on other languages.

You can find the whole code on this branch : https://github.com/benoitjupille/exploration-tools/tree/bug-on-switchcase/tools/src/phone

The files concerned are Phone.h and HangingCall.h.

Thanks !

Pharap · July 26, 2020, 6:37am

After playing around with it for a little while,
I think the most likely explanation is that it’s actually a bug in the compiler.

I’ve pinpointed the issue to the use of the hanging variable.
If I get rid of that variable then it works fine.
E.g.

// Intro has not finished playing
if (needToPlayHangingIntro) {
	needToPlayHangingIntro = hangingCall.introIsPlaying(arduboy, selectedCharacter);
	return;
}

if (arduboy.justPressed(B_BUTTON)) {
	screen = 0;
}

// Hanging animation
if (hangingCall.hanging(arduboy, sound, selectedCharacter))
	return;

screen = 2;

// Populates line buffer with random indexes of alphabet symbols
for (int i=0; i<108; i++) {
	conversationLine[i] = random(0, 30);
}
animationTimer.updatePreviousTime();

Similarly if you wrap that area in an explicit block statement it fixes the problem:

// Intro has not finished playing
if (needToPlayHangingIntro) {
	needToPlayHangingIntro = hangingCall.introIsPlaying(arduboy, selectedCharacter);
	return;
}

// Hanging animation
{
	bool hanging = hangingCall.hanging(
		arduboy,
		sound,
		selectedCharacter
	);

	if (arduboy.justPressed(B_BUTTON)) {
		screen = 0;
	}

	if (hanging) {
		return;
	}
}

screen = 2;

// Populates line buffer with random indexes of alphabet symbols
for (int i=0; i<108; i++) {
	conversationLine[i] = random(0, 30);
}
animationTimer.updatePreviousTime();

The way switch statements work, any variable declared inside any case is actually visible to the other cases, hence you can’t have the same variable declared in more than once case because it would cause a conflict, which is why often an explicit block statement is used for each case statement - to give each case its own scope.

I’d also like to point out something else…

Passing objects like that causes the objects to be copied,
which is wasteful at best, and at worst could cause bugs and odd behaviour.

In particular, any changes to the cursor position, text colour, text background colour, text size and text wrap within the diplay function (or any other function you’re passing arduboy to) will be discarded when the function returns.

To avoid that, use references:

void display(Arduboy2 & arduboy, ArduboyTones & sound)

Even if it isn’t causing a problem, you’re likely to save a fair bit of progmem in the process because passing references is cheaper than copying an Arduboy2 object.

filmote · July 26, 2020, 7:07am

Normally you would get a ‘cross initialisation’ error or warning. Maybe turn warnings on in the compiler?

benoit · July 26, 2020, 7:09am

Hi @Pharap !

Thanks, you helped me a lot ! I don’t think I’d find it by myself. I’ll put it in a another block, that’s good clean idea.

Thanks also for the second point, I was pretty sure it was not the best solution, but now I have a good reason !

See you !

benoit · July 26, 2020, 7:14am

I tried your advice, and you are right, I have a cross initialisation warning on hanging !

That’s not an issue I’m familiar with, I think I’ll check some good practices about that.

Thanks !

Pharap · July 26, 2020, 8:51am

There is indeed a warning, and I would presume that it actually ought to be an error because my nemesis (-fpermissive) has reared its ugly head again:

sketch\src/phone/Phone.h:89:18: warning: jump to case label [-fpermissive]
        case 2:

             ^
sketch\src/phone/Phone.h:65:26: note: crosses initialization of ‘bool hanging’
                bool hanging = hangingCall.hanging(

So it is in fact less a case of a bug with the compiler and more a case of this probably shouldn’t even compile in the first place, but the compiler is allowing it because -fpermissive is supposedly a feature (rather than the bug we all know it really is :P).

I always use an explicit scope so I never really encounter that error.

The easiest solutions are either…
A) Always use an explicit scope:

switch(value)
{
	case Value::A:
		{
			// Do stuff
		}
		break;
		
	case Value::B:
		{
			// Do stuff
		}
		break;
		
	case Value::C:
		{
			// Do stuff
		}
		break;
}

Or B) give each case its own function:

switch(value)
{
	case Value::A:
		handleA();
		break;
		
	case Value::B:
		handleB();
		break;
		
	case Value::C:
		handleC();
		break;
}

Bonus advice:

You also have a few warnings about comparing signed and unsigned values.
Read up on uint8_t, int8_t, uint16_t and int16_t. A good chunk of your ints could easily be uint8_ts because their data range falls within 0 to 255.
Read up on enum class so you can use something like case Screen::ContactList: instead of case 0:.

benoit · July 26, 2020, 9:10am

Thanks again, that’s even more clear now. I’ll look at uint8 and enum things.

filmote · July 26, 2020, 9:23am

There is a nice article on scoped enums in the Arduboy Magazine, Volume 9, Page 29

Pharap · July 27, 2020, 8:02am

26 posts were split to a new topic: Why use switch?

Secret 20% off Arduboy Coupon!

Use coupon code "ERRORCOUPON" at checkout!

Buy Arduboy Now!

Switch doesn't give same results with different case ordering [Solved]