mega: continous twi_listen packets prevented mdb response

Failed to identify root cause. Side-fixed with:
- twi_step will only write response if mdb is in idle state
- successful mdb response will also ship any outgoing twi_data

Important takeaway for mega-client: expect TWI_DATA field in any
response.

Buzzword: priority inversion

Author: temoto

hardware/mega-client/mega.go

@@ -444,18 +444,19 @@ func (self *Client) parse(buf []byte, f *Frame) error {
 		return err
 	}
 
+	for i := 0; i+1 < len(f.Fields.TwiData); i += 2 {
+		twitem := binary.BigEndian.Uint16(f.Fields.TwiData[i : i+2])
+		select {
+		case self.TwiChan <- twitem:
+		default:
+			self.Log.Errorf("CRITICAL TwiChan is full")
+			panic("code error mega TwiChan is full")
+		}
+	}
+
 	switch f.ResponseKind() {
 	case RESPONSE_TWI_LISTEN:
 		atomic.AddUint32(&self.stat.TwiListen, 1)
-		for i := 0; i+1 < len(f.Fields.TwiData); i += 2 {
-			twitem := binary.BigEndian.Uint16(f.Fields.TwiData[i : i+2])
-			select {
-			case self.TwiChan <- twitem:
-			default:
-				self.Log.Errorf("CRITICAL TwiChan is full")
-				panic("code error mega TwiChan is full")
-			}
-		}
 	case RESPONSE_RESET:
 		atomic.AddUint32(&self.stat.Reset, 1)
 		if ResetFlag(f.Fields.Mcusr)&ResetFlagWatchdog != 0 {

hardware/mega-firmware/Makefile

@@ -1,5 +1,5 @@
-mmcu = atmega328p
-# mmcu = atmega128
+# mmcu = atmega328p
+mmcu = atmega168
 
 avr_gcc = avr-gcc
 avr_objcopy = avr-objcopy

hardware/mega-firmware/common.h

@@ -12,6 +12,8 @@
 #define LED_CONFIGURED
 #endif
 
+uint8_t const RESPONSE_RESERVED_FOR_ERROR = 5;
+
 static packet_t volatile request;
 static packet_t volatile response;
 static buffer_t volatile debugb;
@@ -40,6 +42,7 @@ static void spi_init_slave(void);
 static void spi_step(void);
 
 static void twi_init_slave(uint8_t const address);
+static inline void twi_flush_to_response(void);
 static void twi_step(void);
 static void nop(void) __attribute__((used, always_inline));
 
@@ -73,9 +76,8 @@ static inline void debugs(char const *const s) {
 static inline bool response_empty(void) { return response.h.response == 0; }
 
 static bool response_check_capacity(uint8_t const more) {
-  uint8_t const RESERVED_FOR_ERROR = 5;
   // FIXME length check in buffer_append_n is wasted
-  if (response.b.length + more + RESERVED_FOR_ERROR >
+  if (response.b.length + more + RESPONSE_RESERVED_FOR_ERROR >
       PACKET_FIELDS_MAX_LENGTH) {
     buffer_append((buffer_t *)&response.b, FIELD_ERROR2);
     buffer_append_2((buffer_t *)&response.b, ERROR_BUFFER_OVERFLOW, more);

hardware/mega-firmware/config.h

@@ -3,7 +3,7 @@
 
 #define F_CPU 16000000UL  // Clock Speed
 
-#define FIRMWARE_VERSION 0x0200
+#define FIRMWARE_VERSION 0x0201
 #define MDB_TIMEOUT_MS 6
 #define BUFFER_SIZE 50
 

hardware/mega-firmware/main.c

@@ -112,9 +112,7 @@ int main(void) {
     nop();
 
     cli();
-    if (mdb.state != MDB_STATE_IDLE) {
-      mdb_step();
-    }
+    mdb_step();
     sei();
     nop();
 
@@ -138,9 +136,6 @@ static void request_exec(void) {
   } else if (cmd == COMMAND_DEBUG) {
     cmd_debug();
   } else if (cmd == COMMAND_FLASH) {
-    // FIXME simulate bad watchdog event
-    for (;;)
-      ;
     // TODO
     response_error2(ERROR_NOT_IMPLEMENTED, 0);
   } else if (cmd == COMMAND_MDB_BUS_RESET) {

hardware/mega-firmware/mdb.c

@@ -47,6 +47,7 @@ static inline bool uart_send_ready(void) __attribute__((warn_unused_result));
 static void mdb_start_receive(void);
 static void mdb_finish(mdb_result_t const result, uint8_t const error_data);
 static void mdb_bus_reset_finish(void);
+static inline void mdb_handle_done(void);
 static inline void mdb_handle_recv_end(void);
 static inline uint16_t ms_to_timer16_p1024(uint16_t const ms)
     __attribute__((const, warn_unused_result));
@@ -76,28 +77,30 @@ static void mdb_step(void) {
     mdb_handle_recv_end();
   }
   if (mdb.state == MDB_STATE_DONE) {
-    if (!response_empty()) {
-      return;
-    }
+    mdb_handle_done();
+  }
+}
+static inline void mdb_handle_done(void) {
+  if (!response_empty()) {
+    return;
+  }
 
-    uint8_t const r = mdb.result;  // anti-volatile
-    uint8_t len = mdb_in.length;
+  uint8_t const r = mdb.result;  // anti-volatile
+  uint8_t len = mdb_in.length;
 
-    response_begin(RESPONSE_OK);
-    response_f2(FIELD_MDB_RESULT, r, mdb.error_data);
-    response_f2(FIELD_MDB_DURATION10U, (mdb.duration >> 8),
-                (mdb.duration & 0xff));
+  response_begin(RESPONSE_OK);
+  response_f2(FIELD_MDB_RESULT, r, mdb.error_data);
+  response_f2(FIELD_MDB_DURATION10U, (mdb.duration >> 8),
+              (mdb.duration & 0xff));
 
-    if ((r == MDB_RESULT_SUCCESS) && (len > 0)) {
-      len--;
-    }
-    response_fn(FIELD_MDB_DATA, (uint8_t const *const)mdb_in.data, len);
-    response.filled = true;
-    mdb_reset();
-    return;
+  if ((r == MDB_RESULT_SUCCESS) && (len > 0)) {
+    len--;
   }
-
-  return;
+  response_fn(FIELD_MDB_DATA, (uint8_t const *const)mdb_in.data, len);
+  // Convenient place to send TWI_DATA faster than in separate packet.
+  twi_flush_to_response();
+  response.filled = true;
+  mdb_reset();
 }
 static inline void mdb_handle_recv_end(void) {
   uint8_t const len = mdb_in.length;

hardware/mega-firmware/readme.md

@@ -1,6 +1,6 @@
 # What
 
-Firmware for MDB adapter. Later called mega because ATMega328p MCU was used.
+Firmware for MDB adapter. Later called mega because ATMega MCU was used.
 
 Usage patterns:
 - VMC requests: MDB bus reset
@@ -39,6 +39,3 @@ Response (from mega) payload: `response field...`
 `field` uses binary tagged encoding, see `protocol.h` `protocol.go`
 
 Frame with `packet.header=RESET` sent by mega on reboot.
-
-
-# Issues

hardware/mega-firmware/twi.c

@@ -33,12 +33,22 @@ static void twi_step(void) {
     return;
   }
 
-  // TWI session is finished
-  if (response_empty()) {
+  if (response_empty() && (mdb.state == MDB_STATE_IDLE)) {
     response_begin(RESPONSE_TWI_LISTEN);
-    // keyboard sends 1 byte, encode as 2 for future compatibility
     response_fn(FIELD_TWI_DATA, (uint8_t const* const)twi_listen.data, length);
+    buffer_clear_fast((buffer_t * const) & twi_listen);
     response.filled = true;
+  }
+}
+
+static inline void twi_flush_to_response(void) {
+  uint8_t const length = twi_listen.length;  // anti-volatile
+  if (length == 0) {
+    return;
+  }
+  if (response.b.length + length + RESPONSE_RESERVED_FOR_ERROR <
+      PACKET_FIELDS_MAX_LENGTH) {
+    response_fn(FIELD_TWI_DATA, (uint8_t const* const)twi_listen.data, length);
     buffer_clear_fast((buffer_t * const) & twi_listen);
   }
 }
@@ -75,6 +85,7 @@ ISR(TWI_vect) {
     case TW_SR_GCALL_DATA_ACK:
       data = TWDR;
       TWCR = TWCR_ACK;
+      // keyboard sends 1 byte, encode as 2 for future compatibility
       buffer_append_2((buffer_t * const) & twi_listen, 0, data);
       return;