From cefc03d92aa34622424c675c16eca56da8c9f135 Mon Sep 17 00:00:00 2001 From: Daniel Friesel Date: Mon, 22 Feb 2016 22:19:13 +0100 Subject: some more comments --- src/storage.cc | 32 +++++++++++++++++++++++--------- src/storage.h | 24 ++++++++++++++++-------- src/system.cc | 4 +++- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/storage.cc b/src/storage.cc index 2b3ea7c..6a85e22 100644 --- a/src/storage.cc +++ b/src/storage.cc @@ -139,6 +139,11 @@ uint8_t Storage::i2c_receive(uint8_t len, uint8_t *data) } while (!(TWCR & _BV(TWINT))); data[pos] = TWDR; + /* + * No error handling here -- We send the acks, the EEPROM only + * supplies raw data, so there's no way of knowing whether it's still + * talking to us or we're just reading garbage. + */ } return pos; @@ -146,7 +151,7 @@ uint8_t Storage::i2c_receive(uint8_t len, uint8_t *data) /* * Writes len bytes of data into the EEPROM, starting at byte number pos. - * Does not check for page boundaries yet. + * Does not check for page boundaries. * Includes a complete I2C transaction. */ uint8_t Storage::i2c_write(uint8_t addrhi, uint8_t addrlo, uint8_t len, uint8_t *data) @@ -157,18 +162,24 @@ uint8_t Storage::i2c_write(uint8_t addrhi, uint8_t addrlo, uint8_t len, uint8_t addr_buf[0] = addrhi; addr_buf[1] = addrlo; + /* + * The EEPROM might be busy processing a write command, which can + * take up to 10ms. Wait up to 16ms to respond before giving up. + * All other error conditions (even though they should never happen[tm]) + * are handled the same way. + */ for (num_tries = 0; num_tries < 16; num_tries++) { if (num_tries > 0) _delay_ms(1); if (i2c_start_write() != I2C_OK) - continue; + continue; // EEPROM is busy writing if (i2c_send(2, addr_buf) != 2) - continue; + continue; // should not happen if (i2c_send(len, data) != len) - continue; + continue; // should not happen i2c_stop(); return I2C_OK; @@ -180,7 +191,7 @@ uint8_t Storage::i2c_write(uint8_t addrhi, uint8_t addrlo, uint8_t len, uint8_t /* * Reads len bytes of data from the EEPROM, starting at byte number pos. - * Does not check for page boundaries yet. + * Does not check for page boundaries. * Includes a complete I2C transaction. */ uint8_t Storage::i2c_read(uint8_t addrhi, uint8_t addrlo, uint8_t len, uint8_t *data) @@ -191,21 +202,24 @@ uint8_t Storage::i2c_read(uint8_t addrhi, uint8_t addrlo, uint8_t len, uint8_t * addr_buf[0] = addrhi; addr_buf[1] = addrlo; + /* + * See comments in i2c_write. + */ for (num_tries = 0; num_tries < 16; num_tries++) { if (num_tries > 0) _delay_ms(1); if (i2c_start_write() != I2C_OK) - continue; + continue; // EEPROM is busy writing if (i2c_send(2, addr_buf) != 2) - continue; + continue; // should not happen if (i2c_start_read() != I2C_OK) - continue; + continue; // should not happen if (i2c_receive(len, data) != len) - continue; + continue; // should not happen i2c_stop(); return I2C_OK; diff --git a/src/storage.h b/src/storage.h index ea294cb..9eedd52 100644 --- a/src/storage.h +++ b/src/storage.h @@ -26,7 +26,7 @@ class Storage { /** * Enable the storage hardware: Configures the internal I2C - * module and reads the number of stored animations from the + * module and reads the number of stored patterns from the * EEPROM. */ void enable(); @@ -36,7 +36,9 @@ class Storage { * number of stored animations to zero. The next save operation * will get pattern id 0 and overwrite the first stored pattern. * - * This function itself does not write anything to the EEPROM. + * This function does not delete patterns from the EEPROM. However, + * it does reset the pattern counter, thus making all saved patterns + * unavailable to the load and numPatterns functions. */ void reset(); @@ -47,32 +49,38 @@ class Storage { */ bool hasData(); + /** + * Accessor for the number of saved patterns on the EEPROM. + * Only returns valid data if hasData() returned true. + * + * @return number of patterns + */ uint8_t numPatterns() { return num_anims; }; /** * Load pattern from EEPROM. * - * @param idx pattern index + * @param idx pattern index (starting with 0) * @param data pointer to data structure for the pattern. Must be - * at least 256 Bytes + * at least 260 bytes */ void load(uint8_t idx, uint8_t *data); /** - * Save (possibly partial) pattern on the EEPROM. 64 bytes of + * Save (possibly partial) pattern on the EEPROM. 32 bytes of * dattern data will be read and stored, regardless of the * pattern header. * - * @param data pattern data. Must be at least 64 Bytes + * @param data pattern data. Must be at least 32 bytes */ void save(uint8_t *data); /** - * Continue saving a pattern on the EEPROM. Appends 64 bytes of + * Continue saving a pattern on the EEPROM. Appends 32 bytes of * pattern data after the most recently written block of data * (i.e., to the pattern which is currently being saved). * - * @param data pattern data. Must be at least 64 Bytes + * @param data pattern data. Must be at least 32 bytes */ void append(uint8_t *data); }; diff --git a/src/system.cc b/src/system.cc index 19c9849..f472b05 100644 --- a/src/system.cc +++ b/src/system.cc @@ -230,6 +230,8 @@ void System::loop() want_shutdown = 0; } + // TODO refactor the blocks above and below into one + if ((PINC & _BV(PC3)) == 0) { btnMask = (ButtonMask)(btnMask | BUTTON_RIGHT); } @@ -296,5 +298,5 @@ void System::shutdown() ISR(PCINT1_vect) { - // we use PCINT1 for wakeup, so we should catch it here (and do nothing) + // we use PCINT1 for wakeup, so we need an (in this case empty) ISR for it } -- cgit v1.2.3