Bricked help needed [Solved-ish]

We can do some more testing of the flashlight mode, I mean if we really have to disable interrupts, and it works…

But maybe this is sufficient for now, have you been able to confirm if this new version is any better at ensuring functionality?

Yes. I said so above:

Without my current fix, the reset button is required 100% of the time with Sirene. With my fix, flashlight mode has allowed recovery 100% of the times I’ve tried it.

1 Like

The WDT timer has nothing to do with timer0/1. A quick read of the power saving section will show you that. Quite common to shut off timers completely and let WDT wake the chip back up. Doing so is pretty future-proof.

I’m not suggesting disabling interrupts, just turning off the millis/micros timer if that has proved to be an issue. That prevents the need for a tight loop to overwrite memory constantly. In fact your tight loop will actually break things if the timer lands on top of that memory location because the timer could interrupt you INBETWEEN restoring the value so then the time code increments and replaces the value in the middle of your changing it. So at the very least you’d need to wrap the fetch and restore code with stop/enable interrupts for this to work 100%.

Do you have a repository with the code you’re testing?

That’s the issue as it happened with the Sirene game but I don’t know if some other ISR could end up locating a variable at the magic number location and changing it during the bootloader watchdog, and I don’t know if disabling whatever that interrupt or peripheral was for would cause problems.

Rather than doing a thorough investigation of the ramifications of messing with interrupts and peripherals, I though it would be worth trying just hammering the magic number. Although, as you say, there are cases that could cause this method to fail, so far I’ve never had it fail in actual testing. I think for the small percentage of times that it does fail, you just have to try again.

And keep in mind that the chances of even having a critical variable map on to the magic number are very small. Plus, there’s always the reset button as a final solution.

Just a local one at the moment. If you want to examine or test the changes, replace the flashlight() function with this:

#ifndef MAGIC_KEY
#define MAGIC_KEY 0x7777
#endif
#ifndef MAGIC_KEY_POS
#define MAGIC_KEY_POS 0x0800
#endif

void Arduboy2Base::flashlight()
{
  uint16_t savedKey;

  if (!pressed(UP_BUTTON)) {
    return;
  }

  sendLCDCommand(OLED_ALL_PIXELS_ON); // smaller than allPixelsOn()
  digitalWriteRGB(RGB_ON, RGB_ON, RGB_ON);

  // save what's stored at the bootloader key position
  savedKey = *(uint16_t*) MAGIC_KEY_POS;

  while (!pressed(DOWN_BUTTON)) {
    *(volatile uint16_t*) MAGIC_KEY_POS = MAGIC_KEY; // write bootloader key
  }

  // restore saved bootloader key position value
  *(uint16_t*) MAGIC_KEY_POS = savedKey;

  digitalWriteRGB(RGB_OFF, RGB_OFF, RGB_OFF);
  sendLCDCommand(OLED_PIXELS_FROM_RAM);
}

Looks logical. As I said before if you insist on leaving the interrupts on you need to turn them off when retrieving and setting the magic key position to prevent it from being interrupted. That fixes the edge case with the timer, so I can think of no reason not to do that properly.

Other than adding a few more bytes of code :wink:
I’ll consider it.

Upon further thought:

This is necessary in the case of sharing data between foreground code and an ISR. However, in this particular case I think adding disable/enable interrupt code is unnecessary and just takes up extra program space.

The only reason we’re saving and restoring the value at the magic number location is for the situation where that location has been mapped to a global variable not used by an ISR. This allows us to change it to 0x7777 but be able to restore it to the value the sketch expects it to be if a watchdog initiated reboot doesn’t occur and the DOWN button is used to continue the sketch.

If the location is used by an ISR then we really shouldn’t be restoring it at all. In the case of a timer using it, we’d probably end up setting the timer back in time. But, we have no choice because it’s a fixed address and we don’t know how it’s being used. By restoring, we could end up messing it up in a way that the sketch doesn’t behave properly when continued. For such a sketch, you would need to power off and on from flashlight mode instead of being able to safely continue by using the DOWN button.

Maybe we should change flashlight mode back to the way it currently is in the original Arduboy library:
You can’t exit using a button. You just have to reboot. This would eliminate the need to save and restore the magic number value. Plus, we wouldn’t need the code to detect the DOWN button and turn off the LED and screen. The code would be even smaller than it is now.

Is needing to reboot after turning on the flashlight, for it’s intended purpose of actually being a flashlight, much of an inconvenience? Any objections?


As for disabling interrupts while writing the 0x7777:

Without a disable/enable, we could get an interrupt between writing the first and second byte. This might cause the first byte to be overwritten, then if the watchdog causes a reset the magic number will be incorrect.

With a disable/enable, if something would cause an interrupt between writing the first and second byte the interrupt becomes pending. As soon as we enable interrupts both bytes could be overwritten, so again if the watchdog causes a reset the magic number will be incorrect.

There may be a one or two instruction window where the watchdog could trigger and the value would be correct with enable/disable but incorrect without, but I think it’s insignificant compared to the disadvantage of extra enable/disable code needed.

This change would save 50 bytes of program code.

So you’re still suggesting the tight loop that presumes you MIGHT be reseting the device via USB?

I have no issue with killing the ability to exit flashlight mode. Sounds like a great way to get a little space back.

In that case you should really call idle() inside the tight loop thought, might as well get everything from the battery we can. In fact this has the side benefit of letting you set the value right AFTER the timer has fired.

while (true) {
idle(); // resumes when timer fires every ~1ms
// set magic memory location here
}

Your right my suggestion to pause/re-enable interrupts doesn’t help you if interrupts THEN change the value right before WDT hits.

@Cronck I have changed the code a bit, so it now uses a little less RAM. (I moved some variables into constants in PROGMEM)

Now you should be able to compile and upload Sirène again, without having the problem of bricking the upload.

1 Like

I don’t think there is much purpose to exit flashlight mode in software since it can be done in hardware with the power switch.

1 Like