im-tomu / toboot

Bootloader for the EFM32HG Tomu Board
https://tomu.im/
GNU General Public License v3.0
72 stars 28 forks source link

Autorun flag behaviour is opposed to documented one #32

Closed gl-sergei closed 6 years ago

gl-sergei commented 6 years ago

I read following comment:

/// When running a normal program, you won't want Toboot to run.
/// However, when developing new software it is handy to have
/// Toboot run at poweron.  Set this flag to enter Toboot whenever
/// the system has powered on for the first time.
#define TOBOOT_CONFIG_FLAG_AUTORUN_MASK        0x02
#define TOBOOT_CONFIG_FLAG_AUTORUN_SHIFT       1
#define TOBOOT_CONFIG_FLAG_AUTORUN             (1 << 1)

as "if I want to enter toboot on cold boot, I must set config=TOBOOT_CONFIG_FLAG_AUTORUN". But actual behaviour is opposite. I must set config to TOBOOT_CONFIG_FLAG_AUTORUN to disable entering toboot.

Please clarify what is correct behaviour.

xobs commented 6 years ago

You're right, there is a bit of cognitive dissonance here. "Autorun" is ambiguous, because it's not clear what is running -- is it Toboot or the application? I intended for it to be "Auto-run the program on boot".

I'd like it to be opt-in for running applications, and for the default to be to enter Toboot on poweron. This is to make it easy for people who are loading e.g. "blinky" to swap programs.

Do you have a suggestion for what the macro should be called?

kitlith commented 6 years ago

Seems like a comment error, as without the comment "AUTORUN" would imply 'skipping' the bootloader.

i.e. the name and the functionality align, but the comment is wrong or outdated.


EDIT: didn't notice xobs reply before replying

I think the current macro name is fine so long as you fix the documentation and make it clear that booting into toboot is the efault, but if you want to disambiguate it further than TOBOOT_CONFIG_FLAG_SKIP_BOOTLOADER or TOBOOT_CONFIG_FLAG_AUTORUN_APP could work.

gl-sergei commented 6 years ago

I am fine with current behavior and name as long as the comment is fixed. It would also be great to document all supported flags in the API.md.

xobs commented 6 years ago

I have updated API.md and README.md to be clearer. I've also updated comments in the header files, to reflect reality.