Fix STK_SW_MAJOR and STK_SW_MINOR request responses

[ondrej-stanek-ozobot]

The bootloader version is stored at the end of program space,
but the protocol handler falsely uses the optiboot_version address
as an address to RAM, although the data is actually allocated in FLASH
and should be retrieved by a dedicated lpm instructions.

CAUTION: this change increase the binary size 22 bytes on avr-gcc 11.2.0
and 26 bytes on avr-gcc 5.4.0. Therefore, the team might want to
look at different solutions that do not occupy that much code space.
Nonetheless, there is a bug in the current implementation of
STK_SW_MAJOR STK_SW_MINOR that needs addressing.

[ondrej-stanek-ozobot]

The packed attribute might be dropped, as avr-gcc packs everything anyway? Please advise...

[ondrej-stanek-ozobot]

Also, there is reference to the optiboot_version variable on line 845:

        // use rjmp to go around end of flash to address 0
        // it uses fact that optiboot_version constant is 2 bytes before end of flash
        "  rjmp optiboot_version+2\n"

This probably should be replaced with some better constant, as the optiboot_version and its placement at the end of FLASH is just arbitrary and the bootloader code should not rely on it, in my opinion.

[ondrej-stanek-ozobot]

alternatively, much easier fix is:

      if (which == STK_SW_MINOR) {
        putch(OPTIBOOT_MINVER);
      } else if (which == STK_SW_MAJOR) {
        putch(OPTIBOOT_MAJVER + OPTIBOOT_CUSTOMVER);

But then the question is why we need the optiboot_version in the .version section at all?

[WestfW]

the question is why we need the optiboot_version in the .version section at all?

It's there so that the application or a device programmer can easily read it, to check whether calls to do_spm are going to work (for example.)

its placement at the end of FLASH is just arbitrary and the bootloader code should not rely on it

It's not "arbitrary." It's DEFINED to be the last word in flash, precisely so that non-optiboot code can find it.

---

As an "unsigned const int", references to optiboot_version do not become RAM references; they are treated as constant references and loaded with LDI (yes. even though it's in a different section that eventually ends up in a different address space.) Thus the comment about "optimized away."

      /*
       * Send optiboot version as "SW version"
       * Note that the references to memory are optimized away.
       */
      if (which == STK_SW_MINOR) {
        putch(optiboot_version & 0xFF);
    7c88:	11 f0       	breq	.+4      	; 0x7c8e <main+0x8a>
    7c8a:	83 e0       	ldi	r24, 0x03	; 3
      } else if (which == STK_SW_MAJOR) {
        putch(optiboot_version >> 8);
    7c8c:	01 c0       	rjmp	.+2      	; 0x7c90 <main+0x8c>
    7c8e:	88 e0       	ldi	r24, 0x08	; 8
    7c90:	87 d0       	rcall	.+270    	; 0x7da0 <putch>

I'll confess to not remembering why I bothered to define optiboot_version rather than just using the OPTIBOOT_MINVER/etc "obvious" constants.

[ondrej-stanek-ozobot]

As an "unsigned const int", references to optiboot_version do not become RAM references; they are treated as constant references and loaded with LDI

That makes sense, thanks for explaining. I was not aware about the optimization and I falsely assumed this is a bug.

Let me please give some background as to what let me to this PR.
I noticed this part of code as I was adding a new .calibration section right before the .version section. The purpose of the .calibration section is to store the RC oscillator calibration value at defined address in program space (of the bootloader). The calibration value is written by a tool (the data is not present at compile time, obviously). In other words, the .calibration section is burned to the FLASH memory independently. That is when I came to the realization that the mechanism that retrieves the optiboot_version will not work for this particular use case - as the calibration value is not known at compile time - so the lpm instruction have to be used.

The motivation for this endeavor is to always have the internal RC oscillator calibrated, both for the bootloader and for the user application runtime. ( There is an open issue for this: #87 )

Storing the calibration value for the OSCCAL register in FLASH as opposed to EEPROM (which is a common solution) is more robust, as Arduino user is less likely to tamper with bootloader FLASH than with EEPROM. Moreover, the Arduino user doesn't need to think about loading OSCCAL in his code - as it is already taken care of by the bootloader.

[ondrej-stanek-ozobot]

its placement at the end of FLASH is just arbitrary and the bootloader code should not rely on it

It's not "arbitrary." It's DEFINED to be the last word in flash, precisely so that non-optiboot code can find it.

My formulation was bad, sorry. The point is that the core functionality of "launching the user application" depends on another feature "optiboot version can be found at the end of FLASH". The second feature is less important for our application and we wanted to drop it to gain some code space (or to trade it for some other data that would be burned to the end of FLASH).

Better formulation would be: The dependency of the core feature "launching the user application" to a feature "optiboot version can be found at the end of FLASH" is arbitrary. Relaxing this dependency would allow to optionally disable .version section to get more code space, or to replace that section with some other data.

It is not a big deal for us and I can imagine there might be other good reasons why things are done this way.

Thank you very much for your work on optiboot and for prompt responses. Very appreciated!

[WestfW]

Previous to the "rjmp optiboot_version+2" form, the code used an ijmp instruction after loading the vector into registers, making it a couple of instructions longer than the current code. (that code is still present for the VIRTUAL_BOOT case.)
There is a general desire to have as few places as possible know anything about absolute addresses that might change between different parts.)

For calibration, I had somewhat imagined something gross involving a form of self-modifying code so that the final binary would have an "ldi rn, calValue", but I never quite figured out exactly how that would work. (I envisioned code BEFORE the bootloader section that would run the first time the chip went out of reset after burning, and rewrite the appropriate bootloader page using the doSPM feature.) I hadn't noticed how much of a pain it would be to actually use LPM to read a value stored in flash.)

[ondrej-stanek-ozobot]

Rejecting this PR, as it was proven there is no bug to be fixed. The existing implementation produces correct code, as pointed out in comment #342 (comment) .