Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow loading an OSCCAL value from EEPROM #183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matthijskooijman
Copy link
Contributor

When an EEPROM address is given through OSCCAL_EEPROM_ADDR at
compiletime, a byte is read from that address on startup and written to
OSCCAL (unless it is 0xff, which should never be a meaningful
calibration value). This allows improving the accuracy of the UART
baudrate when running off the internal oscillator.

Enabling this option adds 30 bytes on the atmega328p build. This can
probably be reduced by inlining eeprom_read_byte.

@matthijskooijman
Copy link
Contributor Author

Note that I think inlining eeprom_read_byte should probably happen before
merging this PR, but I wanted to get some feedback on the general idea
first, before diving into this (also, I couldn't find the avr-libc
implementation of eeprom_read_byte() just now).

Looking at the assembly output, removing the overhead of the function
call and removing the polling of EEPE (since we're sure no EEPROM
write is in progress) should reduce the cost of this feature to 18 bytes
on an atmega328p.

@matthijskooijman
Copy link
Contributor Author

(Oh, one more reason to not merge yet is that I didn't actually test this code yet, ran out of time for today when the code was about finished and wanted to at least get some feedback before shelving this for a few days)

@WestfW
Copy link
Member

WestfW commented Jun 17, 2016

I was hoping to adjust OSCCAL without using EEPROM, perhaps by merging the calibration with the burning algorithm, and modifying the code on the fly, or something. Figuring out what to use as a clock reference is ... annoying, however :-(

@matthijskooijman
Copy link
Contributor Author

matthijskooijman commented Jun 19, 2016

@WestfW, that's actually a good idea, to store the calibration byte in flash instead of EEPROM. Doing the calibration on the fly (i.e. in the bootloader) will almost certainly be too big to fit in 512 bytes along with the bootloader, but perhaps that's not what you meant.

I've been working on an automated calibration and bootloader flashing tool (running standalone on an Arduino, based on Nick Gammon's Atmega_Board_Programmer sketch), which does calibration by uploading a sketch to the target MCU that calibrates against an external 32kiHz crystal, and writes the result to EEPROM. I can probably modify that sketch to instead write the result through SPI back to the programmer, which can then insert the value into the bootloader image during flash. One common alternative is to let the programmer generate a clock on the MOSI line instead of using a 32kiHz crystal.

As for getting the value into the bootloader, I can see a few approaches:

  1. Create a PROGMEM const that contains a dummy value, and add a bit of code that loads that byte from PROGMEM and stores it to OSCCAL (optionally ignoring a 0xff value so that if no calibration byte was added, no value is written at startup). Setting the actual calibration value is then done by overwriting the PROGMEM variable, whose location can either be automatically be taken from the .elf file, or manually figured out. This approach probably takes 15 bytes of flash.
  2. Add some code that loads a hardcoded value to OSCCAL (unconditionally), and then change the resulting machine code. The compiler would generate a ldi and sts instruction, and the ldi would be modified to change the value to be written. ldi divides the value to load over its two instruction bytes, so this is a bit more complicated than just changing a single byte in the bootloader image. Also, if no calibration byte is set while flashing the bootloader, some hardcoded default byte is written to OSCCAL, which will be worse than the factory calibration in some cases, making this approach less desirable. This approach would be the smallest possible, namely 6 bytes of flash.
  3. Add some NOPs to the bootloader, and while flashing the bootloader replace those nops with the required ldi and sts instructions (with the right calibration value encoded in them). This approach also takes 6 bytes, but has the advantage that if the bootloader is written without any changes, the OSCCAL value is untouched. The downside is that full instructions must be generated by or hardcoded in the bootloader flashing tool, including the (device-dependent) location of the OSCCAL register.
  4. Let the bootloader use pgm_load_word or something similar to load the calibration bytes from the last 2 bytes of flash. This requires that the bootloader compiled size is 2 bytes smaller than the available space. This is essentially the same as option 1, but with a hardcoded location in flash instead of a PROGMEM variable that is placed by the linker.

I'm inclined to go with either option 1 or 3, of which option 1 is probably the easiest and cleanest to implement. Any thoughts?

Apart from the above, one upside of storing the result in EEPROM is that you can reflash the bootloader without having to re-calibrate, since the previous value is just saved. In any case, it seems acceptable to also add code to read OSCCAL from EEPROM (as in this PR now), since there might be cases where EEPROM is preferred over inserting into flash directly. Users can then just select either option based on what they need.

@matthijskooijman
Copy link
Contributor Author

@WestfW, any chance you could have a look at my previous comment? My need for loading calibration bytes from the bootloader has become a bit more urgent again, so I'll be investing some time to implement this soon (but I'd rather implement something that is eligible for merging).

@matthijskooijman
Copy link
Contributor Author

matthijskooijman commented Sep 7, 2016

Oh, just thought of another option, I edited it into the above list.

When an EEPROM address is given through OSCCAL_EEPROM_ADDR at
compiletime, a byte is read from that address on startup and written to
OSCCAL (unless it is 0xff, which should never be a meaningful
calibration value). This allows improving the accuracy of the UART
baudrate when running off the internal oscillator.

Enabling this option adds 30 bytes on the atmega328p build. This can
probably be reduced by inlining eeprom_read_byte.
When the OSCCAL_PROGMEM variable is given at compiletime, a byte is
allocated in PROGMEM (flash) at a fixed location at the top of the flash
space (just below the version bytes, so the third byte from the top).

If this byte is overwritten with a calibration value (e.g. when
programming the bootloader), it will be automatically loaded on startup.
This allows improving the accuracy of the UART baudrate when running off
the internal oscillator, and prevents the actual program from having to
worry about calibration.

The default value of this byte is 0xff, which causes it to be ignored.

To fix the address of this variable, a dedicated .osccal section is used
for it, and the linker is passed some options to put it in the right
place (identical to what happens for the version variable).

Enabling this option adds 15 bytes on the atmega328p build.
@matthijskooijman
Copy link
Contributor Author

I ended up implementation a combination of option 1 and 4. The OSCCAL value is stored in progmem variable defined in the source, but by putting it in a special section (just like the version number), its position is fixed at the end of the flash space (just before the version number, which has not moved). I've added this as a new commit, keeping the EEPROM implementation as well (since I expect that might also be useful for some people, and both can happily coexist).

sleemanj added a commit to sleemanj/optiboot that referenced this pull request Mar 3, 2018
…into tinytuner

This was submitted as a pull request to optiboot's main repository

  * Optiboot#183

See here for why I'm merging it

  * SpenceKonde/ATTinyCore#141 (comment)

untested yet.

Conflicts:
	optiboot/bootloaders/optiboot/Makefile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants