avr: added EEPROM support for ATmega boards

[SoleimanyBen]

This adds general EEPROM support for AVR devices through the EEPROM struct.

First MR here so I was hoping to get feedback on what I could improve and what I did wrong here.

It looks like it goes against convention to not have a Configure() function for hardware structures, but I did not see how It could make sense in this instance.

Also, should there be a specific EEPROM0 constant for each machine file that is then populated within the specific board file?

Thanks again, glad to be able to contribute to such a great project! :)

p.s. im an idiot, sorry about the other PR with the wrong branch selected 😅

[SoleimanyBen]

also now that im thinking about it, maybe the eepromSize constant should also be within board specific files?

edit: I have made changes to this commit so this comment does not apply anymore

[aykevl]

I have a number of nits, but overall the code looks quite reasonable.

Can you also make a PR to https://github.com/tinygo-org/tinygo-site with API documentation on the EEPROM? At least the machine package, but if you could also write something in here (https://tinygo.org/docs/concepts/peripherals/) that would be great.

[aykevl]

This is an unexported function, so it's best to use the most efficient data type available. In this case, int16 (or even uint16).

Suggested change

	func (e EEPROM) setAddress(addr int64) error {
	func (e EEPROM) setAddress(addr int16) error {

[SoleimanyBen]

If I make this change, within this function there is a comparison between an int64 and int16. Should I be casting the int16 into an int64 to perform this check, or should I change the result of the Size() function to return int16 instead?

[aykevl]

In this case you know it will never be bigger than an int16 so casting to the most efficient type is best (for AVR that is int16).

[aykevl]

I believe this constant is part of the .atdf files, but that can be fixed in a future patch. (It needs a change to tools/gen-device-avr).

[SoleimanyBen]

I have a number of nits, but overall the code looks quite reasonable.

Can you also make a PR to https://github.com/tinygo-org/tinygo-site with API documentation on the EEPROM? At least the machine package, but if you could also write something in here (https://tinygo.org/docs/concepts/peripherals/) that would be great.

Absolutely. I'll make a write up once I fix the other issues you mentioned!

[aykevl]

Please define a global constant with this error message to return. For example:

var errEEPROMAddress = errors.New("eeprom: address out of bounds")

Otherwise this forces a heap allocation when the error is triggered, which is not good.

[aykevl]

I think this is an off-by-one.

Suggested change

	if addr < 0 || addr > int16(e.Size()) {
	if addr < 0 || addr >= int16(e.Size()) {

[aykevl]

In this case you know it will never be bigger than an int16 so casting to the most efficient type is best (for AVR that is int16).

[SoleimanyBen]

Sorry for the extreme delay on these changes, will go ahead and add them this week.

[deadprogram]

@SoleimanyBen I was looking at this PR today, and had a couple ideas. Seems to me that it would be great to implement the ReadWriteCloser interface for EEPROM as the main API developers would use.

Imaginary code:

func main() {
    store := machine.EEPROM.Open(address)
    defer store.Close()

    var data [12]byte
    _, err := store.Read(data)
    if err != nil {
        panic("nope")
    }

    println(string(data))


    store.Write([]byte("Hola, mundo!))
}

[SoleimanyBen]

@deadprogram That's a great idea! I'll write up those changes when I make the other adjustments later this week. :)

[aykevl]

@SoleimanyBen I was looking at this PR today, and had a couple ideas. Seems to me that it would be great to implement the ReadWriteCloser interface for EEPROM as the main API developers would use.

Why though? It seems to me that ReaderAt and WriterAt are a much closer fit to the underlying hardware. With ReadWriteClose you have to:

• Open/close the EEPROM, which is not something the hardware needs
• Keep track of the read/write offset, which again isn't natively supported by the hardware

If an application really wanted to layer a ReadWriteCloser on top, you could easily write a new package that wraps the EEPROM to provide these methods. But for the machine API, I think it only adds unnecessary overhead.

[deadprogram]

@SoleimanyBen I think the interface to create here is actually the BlockDevice one, same as https://github.com/tinygo-org/tinyfs/blob/release/tinyfs.go#L42-L69

This is what I have been doing in my Flash PR #3472

[aykevl]

@deadprogram EEPROM is not quite the same as flash though. Unlike flash, you can erase and rewrite individual bytes easily so "page size" (a big part of BlockDevice) doesn't make much sense.

In my opinion, this PR is almost ready, it just needs a few small changes to mergeable.