tinygo icon indicating copy to clipboard operation
tinygo copied to clipboard

avr: added EEPROM support for ATmega boards

Open SoleimanyBen opened this issue 3 years ago • 8 comments

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 :sweat_smile:

SoleimanyBen avatar Nov 21 '22 23:11 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

SoleimanyBen avatar Nov 21 '22 23:11 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!

SoleimanyBen avatar Dec 08 '22 17:12 SoleimanyBen

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

SoleimanyBen avatar Feb 20 '23 17:02 SoleimanyBen

@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!))
}

deadprogram avatar Feb 20 '23 17:02 deadprogram

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

SoleimanyBen avatar Feb 20 '23 17:02 SoleimanyBen

@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.

aykevl avatar Feb 21 '23 15:02 aykevl

@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

deadprogram avatar Feb 26 '23 15:02 deadprogram

@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.

aykevl avatar Apr 27 '23 00:04 aykevl