Add commands to read fuse settings, for SMC diagnostics (14 bytes), version 47.2.4

[stople]

New size: 7076 (max size 7680)

This PR adds the feature of reading fuse settings, to improve diagnostics options when SMC is not working.

To use this feature, add an AUTOBOOT.X16 to root of SD card which prints I2CPEEK at these addresses, and verify that they are what they should (F1 D4 FE). If they are something else (e.g. datasheet default 62 DF FF at 1 MHz), keyboard will not work, but, fuse settings can still be read.

There may be a few optimization options if code size should be kept at a minimum, but, there are still 604 bytes available.

[stople]

The attached AUTOBOOT.X16 (inside smc-diag.zip) will display fuse settings. This have been tested with X16 fuse settings (F1 D4 FE) and with datasheet defult (62 DF FF) (1 MHz, this is what Xgpro programs by default).

To change fuses to datasheet default: avrdude "-CC:\Program Files (x86)\Arduino\hardware\tools\avr/etc/avrdude.conf" -v -pt861 -cstk500v1 -PCOM3 -b19200 -Ulfuse:w:0x62:m -Uhfuse:w:0xDF:m -Uefuse:w:0xFF:m

To change fuses to correct settings: avrdude "-CC:\Program Files (x86)\Arduino\hardware\tools\avr/etc/avrdude.conf" -v -pt861 -cstk500v1 -PCOM3 -b19200 -Ulfuse:w:0xF1:m -Uhfuse:w:0xD4:m -Uefuse:w:0xFE:m

(adjust com port and conf file as needed).

smc-diag.zip

[stople]

For the record: Reading fuses is done using avr-libc function boot_lock_fuse_bits_get(address) (<avr/boot.h>, https://avrdudes.github.io/avr-libc/avr-libc-user-manual-2.2.0/modules.html ). According to the datasheet, it should be ensured that eeprom is not busy when reading fuse bits, hence the eeprom_busy_wait() (<avr/boot.h>). These functions are expected to be done atomically, hence the ATOMIC_BLOCK (<util/atomic.h>)

[stople]

Here is an updated AUTOBOOT.X16, adjusted for the change of order of commands. This will also display lock bits.

Changing order of commands (and parameter optimization) causes FW size to be reduced by 26 bytes.

Tested with lock bits, and with the two specified fuse settings (1 MHz and 16 MHz).

To set lock bits: Add -Ulock:w:0xFC:m

To clear lock bits, erase the chip (e.g. program it with Arduino or ordinary flash write avrdude command)

smc-diag2.zip

[stople]

With both commits, this PR increases code size from 7014 to 7050 bytes (+46 bytes). Max size: 7680.

[stefan-b-jakobsson]

One thought if we are to save some space:

You can't set fuse and lock bits without an external programmer. If you have a programmer, you can get the detailed information about those that way.

Would it suffice, if we had one command offset that basically reports if the settings are correct or not? For that to make sense, the code that reports that must of course turn out to be more compact than the current 46 bytes.

[stople]

One thought if we are to save some space:

You can't set fuse and lock bits without an external programmer. If you have a programmer, you can get the detailed information about those that way.

Would it suffice, if we had one command offset that basically reports if the settings are correct or not? For that to make sense, the code that reports that must of course turn out to be more compact than the current 46 bytes.

If we want this feature, SMC have to read these bytes anyway. Reading fuses takes 8 byte + setting Z pointer.

ldi r24, 9
sts SPMCSR, r24
lpm r24, Z

I just pushed a more optimized version, which seem to only add 26 bytes in total.

• 4 bytes for eeprom safety ref datasheet
• 4 bytes for setting up Z pointer
• 2 bytes for the final rjmp
• 8 bytes "overhead" inside I2C_Send() caused by the new code

For simplifying the I2C interface, we could off course have a single "look for error"-offset. But, then, SMC need to know what is the correct values, have to compare, have to set the correct error if failure etc. Which I think causes the SMC code to become larger. I have chosen to optimize the code for size. A future SMC version could have a "convenience POST offset" (Power On Self Test), an error code which the user can fetch using i2cpeek (if the keyboard works). However, I think the logic (to know which fuses are the good one) can be stored in an AUTOBOOT.X16 instead, so that the user just start the machine with this in the SD-card, and it shows if fuses are bad.

For the sake of argument, here is a possible implementation in ASM:

(store 4 bytes in flash at address X)

ldi r30,0
ldi r31,0
ldi r26,pgmspace
ldi r27,pgmspace
loop:
ldi r24,9
sts SPMCSR, r24
lpm r24,Z+
lpm r25,X+
cmp r24, r25
breq err
cpi r30,4
brlo loop

(also requires error handling / return value / being connected to i2c handling, what I have listed here is 12 instructions + 4 pgm bytes (28 bytes))

[stople]

(Last changes: I removed the atomic block while reading fuse, as this is done inside an interrupt handler. I also made the "request" a local variable, instead of reading it again from I2C_Data[0]. Also added some comments to the readFuse function.)

[stople]

@stefan-b-jakobsson bump. Do you think we can afford using 26 bytes of flash for this feature? In the interest of making it easier to identify fuse related problems? If you do, do you think the I2C offsets used (the range used for these 4 offsets) is a good choice?

I also added this to Readme.md, to try to get better at documenting the stuff I add.

[stefan-b-jakobsson]

Thanks. I've been thinking about it. It's not absolutely necessary that we have this feature, but every now and then a user has a fuse related problem that would benefit from this. I assume we can afford 26 bytes :-)

I haven't tested the code on my hardware. I'll approve it after that.

[stefan-b-jakobsson]

I tested the code now, and I can confirm that it works fine.

If it would be me, I would not use the 0x4... range for these offsets. Maybe there are some more fast read operations in the future that would be nice to have here.

Maybe the 0x3... range or a new 0x5... range would be better?

[stople]

@stefan-b-jakobsson

Strange... Doing some testing, I see that the numerical value of the command affects code size quite significantly (as it is small in the first place).

Original size: 7014 bytes.

If the range is 0x0A..0x0D, code size grows with only 14 bytes (7028). If the range is 0x44..0x47, code size grows with 26 bytes (7040). If the range is 0x94..0x97, code size grows with 34 bytes (7048).

Can we use 0x0A..0x0D, to optimize for size? It will then stay close to the echo/debug offset 0x09, so it should not be that ugly. Yet, these are just numbers anyway, so, who cares what the numerical value is?

[stefan-b-jakobsson]

Yes, that's strange. Could it be that the resulting assembly code for some reason falls out of range for branch instructions when you change the case statements, so that the assembly needs a few extra RJMPs?

Anyway, I was thinking a bit earlier about the system upgrade tool. Verifying that the SMC is write enabled would be a nice preupgrade check. That might an equally important use case, besides isolating SMC issues due to faulty fuse settings.

If we save memory by keeping the offset at the current location, I'm OK with that. Just let me know if that's the way you want to go.

[stople]

If we save memory by keeping the offset at the current location, I'm OK with that. Just let me know if that's the way you want to go.

It was previously 0x44..0x47, I just updated it to 0x0a..0x0d, as that caused code size to end up at 14 bytes, instead of 26 bytes. As flash size is limited, I think we should optimize most of our code for size (but preferably keep the code readable, maintainable and logical). Do you think the range 0x0a..0x0d can be used for this purpose? I have not tested this on the machine yet, but, let me know if you agree to the design and the approach.

[stefan-b-jakobsson]

OK, I misunderstood, and thought it grew by 14 bytes.

I'm OK with 0x0a..0x0d.

[stople]

Sounds good.

Here is an updated autoboot which can show fuse settings if they are incorrect. (I also added a few extra tests as well, making this a "general purpose" SMC diagnostics tool. Marking this tool as preliminary until this PR is merged and a new SMC version have gotten a proper version number.

smc-diag3.zip

[stefan-b-jakobsson]

I tested the last code, and it works fine. I can approve it.

Should we bump the version number first, so we have a way of telling if this functionality is present?

[stople]

I can see that the previous PR also bumped the version number. Thus, I can do the same here, to e.g. 47.2.4.

Do we have any agreed upon convention for how to do version numbers for SMC FW? Bump a version for each new PR? I think it have a value to be able to easily figure out exactly which version you have, and, avoiding that two different version can have the same version number. Like for ROM, where you have releases, prereleases, git hash, and git hash with modification. Which makes it quite traceable, but, adding that may add unneccessary complexity, where we also have not too much flash available. If we do end up with two versions with the same version number, we could as an alternative perform a checksum of flash and compare against known versions, similar to the SMCBLD7.PRG tool.

Should we let the convention be that every PR which changes the machine code, also must bump minimum the 3rd number in the version number?

[stople]

Before this gets merged, maybe adjust the title of the PR, to also mention the version number, so that it gets included in the commit message in the git history. If you agree to the version number 47.2.4, then just mention this version number in the PR header before merge.