SpenceKonde / DxCore

Arduino core for AVR DA, DB, DD, EA and future DU-series parts - Microchip's latest and greatest AVRs. Library maintainers: Porting help and adviccee is available.
Other
189 stars 50 forks source link

EEPROM library not working #26

Closed obdevel closed 4 years ago

obdevel commented 4 years ago

Arduino 1.8.13, macOS 10.13.6

AVR128DA28 DIP package w/Optiboot

EEPROM writes not sticking, e.g.

EEPROM[34] = 10;
Serial.println(EEPROM[34]); // prints 255

I haven't got jtag2updi working so I set the fuses and programmed the bootloader using pyupdi. I based the fuse settings on the avrdude command line displayed by Tools->Burn Bootloader

Everything else seems to be working so far.

Edited to add -------

avr-libc routines don't work either, e.g.

#include <avr/eeprom.h>

  if (eeprom_is_ready()) {
    eeprom_write_byte((uint8_t *)1, 22);
    eeprom_busy_wait();
    Serial.println(eeprom_read_byte((uint8_t *)1));
  }
SpenceKonde commented 4 years ago

Known issue need to port fix from megatinycore


Spence Konde Azzy’S Electronics

New products! Check them out at tindie.com/stores/DrAzzy GitHub: github.com/SpenceKonde ATTinyCore: Arduino support for almost every ATTiny microcontroller Contact: spencekonde@gmail.com

On Fri, Oct 9, 2020, 16:38 obdevel notifications@github.com wrote:

Arduino 1.8.13, macOS 10.13.6

AVR128DA28 DIP package w/Optiboot

EEPROM writes not sticking, e.g.

EEPROM[34] = 10; Serial.println(EEPROM[34]); // prints 255

I haven't got jtag2updi working so I set the fuses and programmed the bootloader using pyupdi. I based the fuse settings on the avrdude command line displayed by Tools->Burn Bootloader

Everything else seems to be working so far.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SpenceKonde/DxCore/issues/26, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTXEW5IO2LMZOPD7Z2RTULSJ5YEXANCNFSM4SKQNGVA .

obdevel commented 4 years ago

Any pointers to how I might solve this myself ? I don't see any relevant appnotes and the one AS Start example isn't helpful either. I've read the NVMCTRL section of the datasheet but I don't know how to convert this to code.

SpenceKonde commented 4 years ago

Hold your horses, it's a 5-minute fix from my end...

SpenceKonde commented 4 years ago

Wait wait wait - the avr-libc routines don't work? Ugh... Okay, that will be a bit longer :-(

SpenceKonde commented 4 years ago

Need to implement those macros, in that case...

SpenceKonde commented 4 years ago

What's in there now is definitely totally hosed though

SpenceKonde commented 4 years ago

I just pushed a version of EEPROM that I would expect to work (in github repo - obv. not in a board manager release).

Does that not work?

SpenceKonde commented 4 years ago

If it doesn't work, please describe how it doesn't work...

obdevel commented 4 years ago

Thanks. I copied that straight over the file in /Users/xxx/Documents/Arduino/hardware/DxCore/megaavr/libraries/EEPROM/src.

Compilation output: [eeprom_test_compile_v1.txt]. I restarted the IDE just to make sure there wasn't anything cached. (https://github.com/SpenceKonde/DxCore/files/5359825/eeprom_test_compile_v1.txt)

Same result - whatever value or address I write reads back as 255, e.g.

EEPROM[0] = 22;
Serial.println(EEPROM[0]);
EEPROM.write(1, 22);
Serial.println(EEPROM.read(1));

But, if it's calling eeprom_read_byte() surely that's resolved from avr-libc ?

SpenceKonde commented 4 years ago

Check NVMCTRL.STATUS after attempting a write, see if there are any error flags.

I suspect read is working correctly, and write is not.

It's easy enough to verify read by just directly reading from the address of the mapped EEPROM

obdevel commented 4 years ago

After a write, NVMCTRL.STATUS is 16. Per the datasheet (sec 10.5.3), that's the LSB of the error code, which translates to 0x1 INVALIDCMD (The selected command is not supported).

SpenceKonde commented 4 years ago

Yup, I had a feeling that was what you'd see....

Yeah, eeprom_write_byte needs to be reimplemented....

obdevel commented 4 years ago

I installed MPLABX and used the mcc to generate some code, but I don't know how to implement this correctly. Maybe this will be a helpful pointer ?

nvmctrl.c.txt

/**
 * \brief Read a byte from eeprom
 *
 * \param[in] eeprom_adr The byte-address in eeprom to read from
 *
 * \return The read byte
 */
uint8_t DA_ReadEepromByte(eeprom_adr_t eeprom_adr)
{
        // Read operation will be stalled by hardware if any write is in progress       
        return *(uint8_t *)(EEPROM_START + eeprom_adr);

}

/**
 * \brief Write a byte to eeprom
 *
 * \param[in] eeprom_adr The byte-address in eeprom to write to
 * \param[in] data The byte to write
 *
 * \return Status of write operation
 */
nvmctrl_status_t DA_WriteEepromByte(eeprom_adr_t eeprom_adr, uint8_t data)
{
        /* Wait for completion of previous write */
        while (NVMCTRL.STATUS & (NVMCTRL_EEBUSY_bm|NVMCTRL_FBUSY_bm));

        /* Program the EEPROM with desired value(s) */
        ccp_write_spm((void *)&NVMCTRL.CTRLA, NVMCTRL_CMD_EEERWR_gc);

        /* Write byte to EEPROM */
        *(uint8_t *)(EEPROM_START + eeprom_adr) = data;

        /* Clear the current command */
        ccp_write_spm((void *)&NVMCTRL.CTRLA, NVMCTRL_CMD_NONE_gc); 

        return NVM_OK;      
}

static inline void ccp_write_spm(void *addr, uint8_t value)
{
    protected_write_io(addr, CCP_SPM_gc, value);
}

There is no implementation provided for protected_write_io() so I assume it's buried somewhere in some library ?

obdevel commented 4 years ago

Ok, this seems to work. Again, cobbled together from disparate MCP sources.

...
#include <avr/xmega.h>
...

/** Datatype for return status of NVMCTRL operations */
typedef enum {
    NVM_OK    = 0, ///< NVMCTRL free, no write ongoing.
    NVM_ERROR = 1, ///< NVMCTRL operation retsulted in error
    NVM_BUSY  = 2, ///< NVMCTRL busy, write ongoing.
} nvmctrl_status_t;

uint8_t da_eeprom_read_byte(uint8_t * index) {
  return *(uint8_t *)(EEPROM_START + index);
}

nvmctrl_status_t da_eeprom_write_byte(uint8_t *index, uint8_t in) {

  /* Wait for completion of previous write */
  while (NVMCTRL.STATUS & (NVMCTRL_EEBUSY_bm|NVMCTRL_FBUSY_bm));

  /* Program the EEPROM with desired value(s) */
  _PROTECTED_WRITE_SPM(NVMCTRL.CTRLA, NVMCTRL_CMD_EEERWR_gc);

  /* Write byte to EEPROM */
  *(uint8_t *)(EEPROM_START + index) = in;

  /* Clear the current command */
  _PROTECTED_WRITE_SPM(NVMCTRL.CTRLA, NVMCTRL_CMD_NONE_gc);

  return NVM_OK;    
}
SpenceKonde commented 4 years ago

540351215f239f2aaf62b2a2d891bd13b23a7e5f .... but yeah your approach is better if that works, somehow I thought (looks like incorrectly) that you weren't supposed to change the command while it was busy....

obdevel commented 4 years ago

I was hoping you'd know better than me ! As I said, it was hacked together using MCC, a couple of app notes and some googling.

Works fine using your changes. Thanks.

SpenceKonde commented 4 years ago

I had a similar problem when I tried to do it in the past defining a macro like that. I was as baffled as you were. That's why I didn't do it like that this time - I'd tripped over it before. No clue why the include guards don't do their job here...

obdevel commented 4 years ago

https://github.com/SpenceKonde/DxCore/blob/master/megaavr/libraries/EEPROM/src/EEPROM.h#L38

Oops :) index needs to be wider than 8 bits to address 512 bytes. Wondered why I was getting strange overwrites.