some more comments

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
 }