stefanrueger / urboot

Small AVR bootloader using urprotocol
GNU General Public License v3.0
55 stars 8 forks source link

Support for UART1 on tiny841, 1634, and some general questions #22

Closed SpenceKonde closed 1 year ago

SpenceKonde commented 1 year ago

So, I have heard that urboot is smaller than optiboot, and does virtual boot erase correctly. If this is the case, I would rather like to adopt this in place of Optiboot as the serial bootloader supplied with ATTinyCore. I am impressed with the variety parts you have builds for (though - I'm rather baffled by the wacky file names. though that can be normalized with a python script - the goal would be to something more like this: urboot_attiny441_8000000ULuart1(more feature flags if applicable).hex (The only operation we can do is concatenation of strings within the context of the platform definition - we have no if/then, no math, just concatenation. And so, I build the hex file names by concatenating build.fcpu, the partnumber, and parameters for options like the LED pin and what port (on the dual usart ones) and such.)

Anyway - I was wondering about a few things. So the 841 and 1634 have 2 hardware serial ports. With optiboot, I provide 3 sets of 841 binaries. One with UART0, one with UART1, and a third with UART0, but the remap register set to move the UART0 to a different set of pins. How hard would support for multiple UARTs be? Is it just a matter of building with different options?

Also, how does Urboot handle the reset flags? My approach has (since I recognized the fatal nature of allowing execution to proceed if there are not reset flags set - I think it causes the majority of hangs that need manual intervention to correct) been to read the reset flags. If they're not 0, copy their current value to GPIOR0, and clear them in the reset flag register (you can't count on the app to do this, the app won't do this, and you;ll think you had a clean reset when in fact it was a dirty reset, and you're trying to use UART at 14400 baud and 1 MHz, except the user code turned off the div8, so now your clock is running 8 times faster than you think, so small wonder the person can't upload even though there's an Optiboot blink. The chip is running the uart at 115200 baud and 8 MHz - things like that - can happen when you smash the return address (conveniently located right at the top of the stack!), or trigger an ISR that didn't exist. while the causes are all bad bugs in their own right, you're always better off bootlooping into a working bootloader than a non-working one. So since I clear the registers, I know that if I see something there, there must have been a reset, as there always must be, so all is well, and if I see nothing, I immediately trigger a reset because that's the only thing that's likely to be successful now. Though the problem is more acute on modern AVRs ( a missing ISR lands you in a state where not only are the settings not at their reset values, everything otherwise appears to work, except that interrupts can't trigger ISRs. (You overflow an array, and then when you return, execution resumes from the start of the program but with most of the current state retained, millis doesn't advance, but micros does, though it just keeps looping through 0-9999... can you imagine trying to debug that mess? On non-bootloader configurations, I have the app do the same thing in .init3).

But yeah, I am very interested in this...

stefanrueger commented 1 year ago

How hard would support for multiple UARTs be

Urboot already supports all UARTs. When compiling you have the options UARTNUM= and, particularly for the t441 and t841, UARTALT=1 to remap the pins. Look, for example, at the autobaud bootloaders for the t841. Fun fact: I've never tried them (don't have the part); you could be the first to try them out! See also the make options for serial comms.

Reset flags

urboot.c clears the MCUSR after putting a copy into R2 (fun fact: ensuring they end up in R2 costs zero bytes code). Urboot starts the application through a WDR (except after flashing through OTA), so R2 will normally see WDRF set and the application cannot really tell whether it's been a genuine WDR or an external reset that caused WDRF via the bootloader:

int main(void) {
  register uint8_t mcusr asm("r2"); // Ask compiler to allocate r2 for reset flags

  asm volatile(" eor r1,r1");   // Clear temporary register r1 to zero
#if !UB_INIT_SP_IS_RAMEND       // Some MCUs initialise SP to 0, not RAMEND, after reset
  SP = RAMEND;
#endif

  // Copy reset flags and clear them
  mcusr = UB_MCUSR;
  UB_MCUSR = 0;
  watchdogConfig(WATCHDOG_OFF);

I actually want GPIOR0 clean on app start for my IDE (not using Arduino) and don't fancy spending two bytes of code for this in the bootloader either. So that will defo be different to what your bootloaders are doing. More discussion on when to start the app and when the bootloader here. I think the way optiboot tries to keep the reset flags is over the top and believe it has whacky side effects.

whacky file names

Sigh, you are catching the urboot.hex repository of pre-compiled bootloaders in a time of change. Am about to change the naming scheme (it's like founding a rock group: 90% of the discussion is about the name). Long short: I want the frequency and baud rate directories on github to list in ascending order of frequency for manual selection. Issue #19 has made me aware of the intricacies of boards.txt files. There are a few ways out for you: as you mentioned you can sprinkle a little python foo over the directory tree to rename the files (or as I would, ln new names into a parallel directory structure); you can adopt @MCUdude's technique; you can compile your own (the makefile has an option MOVETO=... that allows you to set the filename as you fancy); you can ignore all the fcpu/baud rate directories and only use autobaud bootloaders (collapses the number of .hex files by 2-3 orders of magnitude, though that misses out on urboot bootloaders for automotive parts setting the LINUART number of samples to an optimal number in 8..63 to minimise the baud rate quantisation error); ... Anyway, look again after the weekend and the names might be (ever so slightly) less whacky or engage with Issue #19 now to help come to a conclusion.

stefanrueger commented 1 year ago

something more like this: urboot_attiny441_8000000ULuart1(more feature flags if applicable).hex

I noticed the UL suffix. Take note that urboot.c relies on F_CPU to be signed; so if you compile your own ensure you pass the option F_CPU=xyzL not F_CPU=xyzUL. I have not reviewed the code whether signedness of F_CPU matters but it is assumed to be signed in the SWIO code: urboot ensures processing an rx/tx bit costs F_CPU/BAUD_RATE cycles within 0.5 cycles (optiboot in contrast uses Atmel AN 305, which only comes within +/- 3 cycles, ie, has much higher quantisation error). The pre-processor computations involve comparisons that might not work when F_CPU is unsigned.

SpenceKonde commented 1 year ago

The problem with stashing it in R2 is that you *aren't guaranteed that it will still be there by the time all the init sections have been run plus the core init functions.... There could be some innocuous looking class with a doozie of a constructor that hogs a great number of registers, and that could squeeze it out before we get to user code. The compiler has every right to use r2 as it sees fit right? So we're just hoping that the way registers are allocated for constructors and initialization code won't smash R2. I don't think that's a valid assumption. It is probably true in simple test situations, but I'm not confident it would work in a full scale system, especially not if some poorly written and inefficient arduino libraries (most of them) are thrown in..

stefanrueger commented 1 year ago

Yes, all correct observations about the use of R2 for the MCUSR copy. Applications that use this feature better make use of the .init0 section to copy R2 into a variable before all other initialisations happen (optiboot has published working example code for that). You are also correct that asm("r2") is advisory only, though in this case the compiler does the source's bidding... One trick of the trade for tight code is to stick with the same compiler. I only use avr-gcc 4.8.1, and occasionally, 5.4.0 for their particularly tight code.

The urboot project specialises in small bootloaders and, as such, finds it hard to justify the use of 2 bytes code in every b/loader for stashing something in GPIOR0. The other problem is that were the bootloader to taint GPIOR0, then every application wanting to use GPIOR0 would need clear it before use: not only are another 2 bytes in those applications gone, the unsuspecting user reading the data sheet would not expect GPIOR0 to be uninitialised after reset.

My application code regularly uses GPIOR0 for eight slick global Boolean variables. Here from a header file in my IDE:

/*
 * Provide eight global Boolean variables through GPIOR0, which normally
 * reside in I/O register space, so the compiler can make use of efficient
 * bit addressable cbi/sbi instructions as well as sbrs/sbrc tests. Just, eg,
 *
 * #define mybool0 board_bool(0)
 * #define mybool1 board_bool(1)
 *
 * and use mybool0 and mybool1 as global Boolean variables.
 */

typedef struct { uint8_t b0:1, b1:1, b2:1, b3:1, b4:1, b5:1, b6:1, b7:1; } board_bool8_t;
#ifdef GPIOR0
#define board_bool8 GPIOR0
#define board_bool(x) (((board_bool8_t *)_SFR_MEM_ADDR(GPIOR0))-> b ## x)
#else
extern board_bool8_t board_bool8;
#define board_bool(x) (((board_bool8_t *)&board_bool8)-> b ## x)
#endif
stefanrueger commented 1 year ago

But anyway, seems like you have a lot of experience and have serious bootloader deployments, so I guess you can do proper field testing with these nifty little urboot bootloaders.

SpenceKonde commented 1 year ago

Does urboot recognize that if there are no flags set, that the peripherals cannot be assumed to be in any state? On modern AVRs, this is straightforward to fix - you immediately issue a software reset (on modern AVRs, the consequences of not doing so are much worse - not only are the peripherals more complex, and hence potentially in more complicated undesired configurations, the LVL1EX bit may be set in CPUINT, because of the way BAD_ISR is defined. Since no RETI is ever executed, after a bad interrupt, even if everything else worked, it would still leave the chip in a non-functional state, because interrupts would be disabled because it thinks it's still in one, and there isn't a way to unpick this other than resetting the chip.

The reason I use GPIOR0 is that getting the flags out of R2 is not something you can explain to someone how to do in 2 lines of code (uint8_t flags = GPIOR0; GPIOR0 = 0; This is Arduino-land, 95% of the population doesn't know what a section is.... I looked at the R2 strategy, assessed that using it was going to be beyond a large portion of users, and replaced it with the GPIOR method, on the grounds that doing this would lead to fewer people having difficulty figuring out how to get the reset flags. I use the remaining two bits of GPIOR0 anyway to record critical issues with the configuration of tuned clocks (we never touch GPIOR after startup). I make extensive use of the GPIORs too - but I still consider this preferable to

stefanrueger commented 1 year ago

Does urboot recognize that if there are no flags set, that the peripherals cannot be assumed to be in any state?

Urboot hands over control to the application in this case. What else do you want it to do here? It has no idea what the cause of "no flags" is. It could be

I don't see what the bootloader could (or should) do otherwise.

On modern AVRs, this is straightforward to fix - you immediately issue a software reset

Urboot hasn't been extended to modern parts (yet). Might consider that when I get round to do that, but it will need some more persuasion to get the bootloader to do that. Why wouldn't the application's (probably weakly defined and therefore overridable) __bad_interrupt routine do that? Sounds like that would be the correct place.

getting the flags out of R2 is not something you can explain

You don't have to! Your core is free to put into main.c' a two-byte instruction at .init0 to the effect of

asm volatile("out %[gpior0], r2\n" :: [gpior0] "I"(_SFR_IO_ADDR(GPIOR0)));

That way GPIOR0 is available in your cores just like you want it to. Maybe you find a way of doing that conditionally, so that that code only exists if the application needs to know about the reset flags. (Mine never needed that.)

Mind you, GPIOR0 isn't available in all classic AVRs. For example the venerable ATmega32 doesn't have that.

All things considered, I still think it is a bad idea for the bootloader to use GPIOR0. How would a user know they cannot rely on GPIOR0 being initialised with 0 by reset? They wouldn't expect that, would they? The bootloader is agnostic to which IDE, core, programming language, "operating system" is used and shouldn't interfere more than needed.

stefanrueger commented 1 year ago

@SpenceKonde I guess we have discussed all you wanted to know? LMK if there are still questions unanswered (either re-open the issue or open a new one).