Always use pullup with ps2 logic, except when PSU is off (#41)

* GPIO library: Add dedicated inputWithPullup/driveLow, for PS2

This is to simplify toggling between the two meaningful states: Input with
pullup, and output driving low.

This is particularly useful for PS2, which is open collector logic,
requiring a strong pullup. The external 4.7k pullup is not always strong
enough, but together with the 20-50k additional internal pullup, it handles
all known keyboards.

* Use the new inputWithPullup/driveLow functions in PS2 logic

* Disable PS2 pullup when power supply is off

This is to prevent trying to power the keyboard via the PS2 ports when the
power supply is off.

* Bump version to 47.2.1

optimized_gpio.c

@@ -69,15 +69,15 @@ static void pinMode_opt_params_not_known_compiletime(uint8_t pin, uint8_t mode)
   uint8_t tmp = SREG;
   cli();
   if (mode == INPUT) {
-    *ddr_reg &= ~bitmask;
-    *port_reg &= ~bitmask;
+    *ddr_reg &= ~bitmask;  // Intermediate step -> Input + (high Z or pullup)
+    *port_reg &= ~bitmask; // Input + High Z
   }
   else if (mode == INPUT_PULLUP) {
-    *ddr_reg &= ~bitmask;
-    *port_reg |= bitmask;
+    *ddr_reg &= ~bitmask; // Intermediate step -> Input + (high Z or pullup)
+    *port_reg |= bitmask; // Input + pullup
   }
   else {
-    *ddr_reg |= bitmask;
+    *ddr_reg |= bitmask; // Output, high/low is undefined
   }
   SREG = tmp;
 }
@@ -176,3 +176,42 @@ inline uint8_t digitalRead_opt(uint8_t pin)
     return digitalRead_opt_params_not_known_compiletime(pin);
   }
 }
+
+
+inline void gpio_inputWithPullup(uint8_t pin)
+{
+  // pinMode_opt changes both ddr and port when setting to INPUT_PULLUP
+  pinMode_opt(pin, INPUT_PULLUP);
+}
+
+
+// If pin is not known compile-time, the inlined gpio_driveLow will call this non-inlined function
+static void gpio_driveLow_params_not_known_compiletime(uint8_t pin) __attribute__((noinline));
+static void gpio_driveLow_params_not_known_compiletime(uint8_t pin)
+{
+  volatile uint8_t *ddr_reg = get_ddr_address_from_pin(pin);
+  volatile uint8_t *port_reg = get_port_address_from_pin(pin);
+  uint8_t bitmask = get_bitmask_from_pin(pin);
+  uint8_t tmp = SREG;
+  cli();
+  *port_reg &= ~bitmask; // Set port LOW -> Intermediate step, either high Z or driving pin low
+  *ddr_reg |= bitmask;   // Set DDR HIGH -> Drive pin low
+  SREG = tmp;
+}
+
+inline void gpio_driveLow(uint8_t pin)
+{
+  if (__builtin_constant_p(pin)) {
+    // pin and val is known at compile time.
+    // This will be inlined to two cbi/sbi instructions.
+    digitalWrite_opt(pin, LOW);
+    pinMode_opt(pin, OUTPUT);
+  }
+  else
+  {
+    // pin is not known at compile time.
+    // Implementation cannot use cbi/sbi. Hence, atomic pin manipulation is required.
+    // Call a non-inlined function from this inline function.
+    gpio_driveLow_params_not_known_compiletime(pin);
+  }
+}

optimized_gpio.h

@@ -8,10 +8,15 @@
 extern "C" {
 #endif
 
+// Arduino-style interface
 extern inline void pinMode_opt(uint8_t pin, uint8_t mode) __attribute__((always_inline));
 extern inline void digitalWrite_opt(uint8_t pin, uint8_t val) __attribute__((always_inline));
 extern inline uint8_t digitalRead_opt(uint8_t pin) __attribute__((always_inline));
 
+// Convenience functions to choose between input+pullup and output+low, needed by e.g. PS2
+extern inline void gpio_inputWithPullup(uint8_t pin) __attribute__((always_inline));
+extern inline void gpio_driveLow(uint8_t pin) __attribute__((always_inline));
+
 #ifdef __cplusplus
 }
 #endif

ps2.h

@@ -4,6 +4,8 @@
 #include "optimized_gpio.h"
 #define SCANCODE_TIMEOUT_MS 50
 
+bool PWR_ON_active();
+
 enum PS2_CMD_STATUS : uint8_t {
   IDLE = 0,
   CMD_PENDING = 1,
@@ -44,8 +46,15 @@ class PS2Port
     };
 
     virtual void resetInput() {
-      pinMode_opt(datPin, INPUT);
-      pinMode_opt(clkPin, INPUT);
+      if (PWR_ON_active()) {
+        gpio_inputWithPullup(datPin);
+        gpio_inputWithPullup(clkPin);
+      } else {
+        // Prevent powering the keyboard via the pull-ups when system is off
+        // Call reset() after changing PWR_ON
+        pinMode_opt(datPin, INPUT);
+        pinMode_opt(clkPin, INPUT);
+      }
       curCode = 0;
       parity = 0;
       rxBitCount = 0;
@@ -173,12 +182,10 @@ class PS2Port
         case 7:
           //Output data bits 0-7
           if (outputBuffer[0] & 1) {
-            pinMode_opt(datPin, INPUT);
-            digitalWrite_opt(datPin, HIGH);
+            gpio_inputWithPullup(datPin);
           }
           else {
-            digitalWrite_opt(datPin, LOW);
-            pinMode_opt(datPin, OUTPUT);
+            gpio_driveLow(datPin);
           }
 
           //Update parity
@@ -194,12 +201,10 @@ class PS2Port
         case 8:
           //Send odd parity bit
           if ((parity & 1) == 1) {
-            digitalWrite_opt(datPin, LOW);
-            pinMode_opt(datPin, OUTPUT);
+            gpio_driveLow(datPin);
           }
           else {
-            pinMode_opt(datPin, INPUT);
-            digitalWrite_opt(datPin, HIGH);
+            gpio_inputWithPullup(datPin);
           }
 
           //Prepare for stop bit
@@ -208,8 +213,7 @@ class PS2Port
 
         case 9:
           //Stop bit
-          pinMode_opt(datPin, INPUT);
-          digitalWrite_opt(datPin, HIGH);
+          gpio_inputWithPullup(datPin);
           rxBitCount++;
           break;
 
@@ -291,19 +295,16 @@ class PS2Port
 
       else if (timerCountdown == 3) {
         //Initiate request-to-send sequence
-        digitalWrite_opt(clkPin, LOW);
-        pinMode_opt(clkPin, OUTPUT);
-
-        digitalWrite_opt(datPin, LOW);
-        pinMode_opt(datPin, OUTPUT);
+        gpio_driveLow(clkPin);
+        gpio_driveLow(datPin);
 
         ps2ddr = 1;
         timerCountdown--;
       }
 
       else if (timerCountdown == 1) {
         //We are at end of the request-to-send clock hold time of minimum 100 us
-        pinMode_opt(clkPin, INPUT);
+        gpio_inputWithPullup(clkPin);
 
         timerCountdown = 0;
         rxBitCount = 0;

version.h

@@ -1,3 +1,3 @@
 #define version_major 47
 #define version_minor 2
-#define version_patch 0
+#define version_patch 1

x16-smc.ino

@@ -185,8 +185,8 @@ void setup() {
 
   // Setup Power Supply
   pinMode_opt(PWR_OK, INPUT);
-  pinMode_opt(PWR_ON, OUTPUT);
   digitalWrite_opt(PWR_ON, HIGH);
+  pinMode_opt(PWR_ON, OUTPUT);
 
   // Turn Off Activity LED
   pinMode_opt(ACT_LED, OUTPUT);
@@ -337,6 +337,8 @@ void PowerOffSeq() {
   digitalWrite_opt(ACT_LED, ACT_LED_OFF);     // Ensure activity LED is off
   _delay_ms(AUDIOPOP_HOLDTIME_MS);                // Wait for audio system to stabilize before power is turned off
   digitalWrite_opt(PWR_ON, HIGH);             // Turn off supply
+  Keyboard.reset();                           // Reset and deactivate pullup
+  Mouse.reset();                              // Reset and deactivate pullup
   SYSTEM_POWERED = 0;                         // Global Power state Off
   _delay_ms(RESB_HOLDTIME_MS);                    // Mostly here to add some delay between presses
   deassertReset();
@@ -345,6 +347,8 @@ void PowerOffSeq() {
 void PowerOnSeq() {
   assertReset();
   digitalWrite_opt(PWR_ON, LOW);              // turn on power supply
+  Keyboard.reset();                           // Reset and activate pullup
+  Mouse.reset();                              // Reset and activate pullup
   unsigned long TimeDelta = 0;
   unsigned long StartTime = millis();         // get current time
   while (!digitalRead_opt(PWR_OK)) {          // Time how long it takes
@@ -356,8 +360,6 @@ void PowerOnSeq() {
     // insert error handler, flash activity light & Halt?   IE, require hard power off before continue?
   }
   else {
-    Keyboard.flush();
-    Mouse.reset();
     defaultRequest = I2C_CMD_GET_KEYCODE_FAST;
     _delay_ms(RESB_HOLDTIME_MS);                // Allow system to stabilize
     SYSTEM_POWERED = 1;                     // Global Power state On
@@ -689,3 +691,20 @@ ISR(TIMER1_COMPA_vect) {
   Keyboard.timerInterrupt();
   Mouse.timerInterrupt();
 }
+
+bool PWR_ON_active()
+{
+  // Returns the status of the PWR_ON output port.
+  // PWR_ON is active low.
+  // Thus, return true if DDR is output (1) and if PORT is low (0).
+  // Similar to the SYSTEM_POWERED variable, but this is more accurate when power is changing.
+  // A future code improvement can be to make gpio functions to read from PORT/DDR register
+
+  // The following code assumes PWR_ON is pin 5, which is PA5.
+#if PWR_ON != 5
+  #error Please adjust PWR_ON_active()
+#endif
+
+  if ((DDRA & _BV(5)) == 0) return false; // Port is input. This is the case when mouse and keyboard objects are created
+  return (PORTA & _BV(5)) ? false : true;
+}