From d20909db1050017d6ae15d6f10ad48ecc8ee842d Mon Sep 17 00:00:00 2001
From: Christopher Mullins <chris@sidoh.org>
Date: Sun, 30 Jun 2019 22:21:05 -0700
Subject: [PATCH 1/5] Pull out reverseBits into shared lib

---
 lib/Radio/RadioUtils.cpp | 18 ++++++++++++++++++
 lib/Radio/RadioUtils.h   |  8 ++++++++
 2 files changed, 26 insertions(+)
 create mode 100644 lib/Radio/RadioUtils.cpp
 create mode 100644 lib/Radio/RadioUtils.h

diff --git a/lib/Radio/RadioUtils.cpp b/lib/Radio/RadioUtils.cpp
new file mode 100644
index 00000000..aa97a144
--- /dev/null
+++ b/lib/Radio/RadioUtils.cpp
@@ -0,0 +1,18 @@
+#include <RadioUtils.h>
+
+#include <stdint.h>
+#include <stddef.h>
+#include <Arduino.h>
+
+uint8_t reverseBits(uint8_t byte) {
+  uint8_t result = byte;
+  uint8_t i = 7;
+
+  for (byte >>= 1; byte; byte >>= 1) {
+    result <<= 1;
+    result |= byte & 1;
+    --i;
+  }
+
+  return result << i;
+}
\ No newline at end of file
diff --git a/lib/Radio/RadioUtils.h b/lib/Radio/RadioUtils.h
new file mode 100644
index 00000000..f909bec8
--- /dev/null
+++ b/lib/Radio/RadioUtils.h
@@ -0,0 +1,8 @@
+#pragma once
+
+#include <stdint.h>
+
+/**
+ * Reverse the bits of a given byte
+ */
+uint8_t reverseBits(uint8_t byte);
\ No newline at end of file

From b9230357a2c0ac99a1ae6174595d8712a9478ae8 Mon Sep 17 00:00:00 2001
From: Christopher Mullins <chris@sidoh.org>
Date: Sun, 30 Jun 2019 22:25:02 -0700
Subject: [PATCH 2/5] Remove unused settings

---
 lib/Radio/NRF24MiLightRadio.cpp | 14 --------------
 lib/Radio/PL1167_nRF24.cpp      | 15 +--------------
 lib/Radio/PL1167_nRF24.h        |  3 ---
 3 files changed, 1 insertion(+), 31 deletions(-)

diff --git a/lib/Radio/NRF24MiLightRadio.cpp b/lib/Radio/NRF24MiLightRadio.cpp
index f16d4c4c..b8efc3ba 100644
--- a/lib/Radio/NRF24MiLightRadio.cpp
+++ b/lib/Radio/NRF24MiLightRadio.cpp
@@ -35,20 +35,6 @@ int NRF24MiLightRadio::begin() {
 }
 
 int NRF24MiLightRadio::configure() {
-  int retval = _pl1167.setCRC(true);
-  if (retval < 0) {
-    return retval;
-  }
-
-  retval = _pl1167.setPreambleLength(3);
-  if (retval < 0) {
-    return retval;
-  }
-
-  retval = _pl1167.setTrailerLength(4);
-  if (retval < 0) {
-    return retval;
-  }
 
   retval = _pl1167.setSyncword(_config.syncword0, _config.syncword3);
   if (retval < 0) {
diff --git a/lib/Radio/PL1167_nRF24.cpp b/lib/Radio/PL1167_nRF24.cpp
index 72b2ec2d..caa5b9e9 100644
--- a/lib/Radio/PL1167_nRF24.cpp
+++ b/lib/Radio/PL1167_nRF24.cpp
@@ -55,21 +55,8 @@ int PL1167_nRF24::recalc_parameters()
   _radio.setChannel(2 + _channel);
 
   _radio.setPayloadSize( packet_length );
-  return 0;
-}
-
-
-int PL1167_nRF24::setPreambleLength(uint8_t preambleLength)
-{ return 0; }
-/* kh- no thanks, I'll take care of this */
 
-
-int PL1167_nRF24::setSyncword(uint16_t syncword0, uint16_t syncword3)
-{
-  _syncwordLength = 5;
-  _syncword0 = syncword0;
-  _syncword3 = syncword3;
-  return recalc_parameters();
+  return 0;
 }
 
 int PL1167_nRF24::setTrailerLength(uint8_t trailerLength)
diff --git a/lib/Radio/PL1167_nRF24.h b/lib/Radio/PL1167_nRF24.h
index 42166004..408924f9 100644
--- a/lib/Radio/PL1167_nRF24.h
+++ b/lib/Radio/PL1167_nRF24.h
@@ -33,11 +33,8 @@ class PL1167_nRF24 {
   private:
     RF24 &_radio;
 
-    bool _crc;
-    uint8_t _preambleLength = 1;
     uint16_t _syncword0 = 0, _syncword3 = 0;
     uint8_t _syncwordLength = 4;
-    uint8_t _trailerLength = 4;
     uint8_t _maxPacketLength = 8;
 
     uint8_t _channel = 0;

From c5d9174be984990f3b29abfbc401cd030863e2bd Mon Sep 17 00:00:00 2001
From: Christopher Mullins <chris@sidoh.org>
Date: Sun, 30 Jun 2019 22:26:21 -0700
Subject: [PATCH 3/5] Pre-compute syncword bytes

---
 lib/Radio/MiLightRadioConfig.cpp |  8 +++----
 lib/Radio/MiLightRadioConfig.h   | 33 ++++++++++++++++++++++------
 lib/Radio/NRF24MiLightRadio.cpp  |  3 +--
 lib/Radio/PL1167_nRF24.cpp       | 37 +++++++++++---------------------
 4 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/lib/Radio/MiLightRadioConfig.cpp b/lib/Radio/MiLightRadioConfig.cpp
index bd47b9f0..b58c872c 100644
--- a/lib/Radio/MiLightRadioConfig.cpp
+++ b/lib/Radio/MiLightRadioConfig.cpp
@@ -1,8 +1,8 @@
 #include <MiLightRadioConfig.h>
 
 MiLightRadioConfig MiLightRadioConfig::ALL_CONFIGS[] = {
-  MiLightRadioConfig(0x147A, 0x258B, 7, 9, 40, 71), // rgbw
-  MiLightRadioConfig(0x050A, 0x55AA, 7, 4, 39, 74), // cct
-  MiLightRadioConfig(0x7236, 0x1809, 9, 8, 39, 70), // rgb+cct, fut089
-  MiLightRadioConfig(0x9AAB, 0xBCCD, 6, 3, 38, 73)  // rgb
+  MiLightRadioConfig(0x147A, 0x258B, 7, 9, 40, 71, 0xAA, 0x05), // rgbw
+  MiLightRadioConfig(0x050A, 0x55AA, 7, 4, 39, 74, 0xAA, 0x05), // cct
+  MiLightRadioConfig(0x7236, 0x1809, 9, 8, 39, 70, 0xAA, 0x05), // rgb+cct, fut089
+  MiLightRadioConfig(0x9AAB, 0xBCCD, 6, 3, 38, 73, 0x55, 0x0A)  // rgb
 };
diff --git a/lib/Radio/MiLightRadioConfig.h b/lib/Radio/MiLightRadioConfig.h
index 7b8fc0c8..54871667 100644
--- a/lib/Radio/MiLightRadioConfig.h
+++ b/lib/Radio/MiLightRadioConfig.h
@@ -1,6 +1,7 @@
 #include <Arduino.h>
 #include <MiLightRemoteType.h>
 #include <Size.h>
+#include <RadioUtils.h>
 
 #ifndef _MILIGHT_RADIO_CONFIG
 #define _MILIGHT_RADIO_CONFIG
@@ -10,6 +11,7 @@
 class MiLightRadioConfig {
 public:
   static const size_t NUM_CHANNELS = 3;
+  static const uint8_t SYNCWORD_LENGTH = 5;
 
   MiLightRadioConfig(
     const uint16_t syncword0,
@@ -17,20 +19,37 @@ class MiLightRadioConfig {
     const size_t packetLength,
     const uint8_t channel0,
     const uint8_t channel1,
-    const uint8_t channel2
-  )
-    : syncword0(syncword0),
-      syncword3(syncword3),
-      packetLength(packetLength)
+    const uint8_t channel2,
+    const uint8_t preamble,
+    const uint8_t trailer
+  ) : syncword0(syncword0)
+    , syncword3(syncword3)
+    , packetLength(packetLength)
   {
     channels[0] = channel0;
     channels[1] = channel1;
     channels[2] = channel2;
+
+    size_t ix = SYNCWORD_LENGTH;
+
+    // precompute the syncword for the nRF24.  we include the fixed preamble and trailer in the
+    // syncword to avoid needing to bitshift packets.  trailer is 4 bits, so the actual syncword
+    // is no longer byte-aligned.
+    syncwordBytes[ --ix ] = reverseBits(
+      ((syncword0 << 4) & 0xF0) | (preamble & 0x0F)
+    );
+    syncwordBytes[ --ix ] = reverseBits((syncword0 >> 4) & 0xFF);
+    syncwordBytes[ --ix ] = reverseBits(((syncword0 >> 12) & 0x0F) + ((syncword3 << 4) & 0xF0));
+    syncwordBytes[ --ix ] = reverseBits((syncword3 >> 4) & 0xFF);
+    syncwordBytes[ --ix ] = reverseBits(
+      ((syncword3 >> 12) & 0x0F) | ((trailer << 4) & 0xF0)
+    );
   }
 
-  const uint16_t syncword0;
-  const uint16_t syncword3;
   uint8_t channels[3];
+  uint8_t syncwordBytes[SYNCWORD_LENGTH];
+  uint16_t syncword0, syncword3;
+
   const size_t packetLength;
 
   static const size_t NUM_CONFIGS = 4;
diff --git a/lib/Radio/NRF24MiLightRadio.cpp b/lib/Radio/NRF24MiLightRadio.cpp
index b8efc3ba..b2fefc9f 100644
--- a/lib/Radio/NRF24MiLightRadio.cpp
+++ b/lib/Radio/NRF24MiLightRadio.cpp
@@ -35,8 +35,7 @@ int NRF24MiLightRadio::begin() {
 }
 
 int NRF24MiLightRadio::configure() {
-
-  retval = _pl1167.setSyncword(_config.syncword0, _config.syncword3);
+  int retval = _pl1167.setSyncword(_config.syncwordBytes, MiLightRadioConfig::SYNCWORD_LENGTH);
   if (retval < 0) {
     return retval;
   }
diff --git a/lib/Radio/PL1167_nRF24.cpp b/lib/Radio/PL1167_nRF24.cpp
index caa5b9e9..c3ed1025 100644
--- a/lib/Radio/PL1167_nRF24.cpp
+++ b/lib/Radio/PL1167_nRF24.cpp
@@ -11,6 +11,8 @@
  */
 
 #include "PL1167_nRF24.h"
+#include <RadioUtils.h>
+#include <MiLightRadioConfig.h>
 
 static uint16_t calc_crc(uint8_t *data, size_t data_length);
 static uint8_t reverse_bits(uint8_t data);
@@ -26,7 +28,7 @@ int PL1167_nRF24::open()
   _radio.setDataRate(RF24_1MBPS);
   _radio.disableCRC();
 
-  _syncwordLength = 5;
+  _syncwordLength = MiLightRadioConfig::SYNCWORD_LENGTH;
   _radio.setAddressWidth(_syncwordLength);
 
   return recalc_parameters();
@@ -37,39 +39,26 @@ int PL1167_nRF24::recalc_parameters()
   int packet_length = _maxPacketLength + 2;
   int nrf_address_pos = _syncwordLength;
 
-  if (_syncword0 & 0x01) {
-    _nrf_pipe[ --nrf_address_pos ] = reverse_bits( ( (_syncword0 << 4) & 0xf0 ) + 0x05 );
-  } else {
-    _nrf_pipe[ --nrf_address_pos ] = reverse_bits( ( (_syncword0 << 4) & 0xf0 ) + 0x0a );
+  if (packet_length > sizeof(_packet) || nrf_address_length < 3) {
+    return -1;
   }
-  _nrf_pipe[ --nrf_address_pos ] = reverse_bits( (_syncword0 >> 4) & 0xff);
-  _nrf_pipe[ --nrf_address_pos ] = reverse_bits( ( (_syncword0 >> 12) & 0x0f ) + ( (_syncword3 << 4) & 0xf0) );
-  _nrf_pipe[ --nrf_address_pos ] = reverse_bits( (_syncword3 >> 4) & 0xff);
-  _nrf_pipe[ --nrf_address_pos ] = reverse_bits( ( (_syncword3 >> 12) & 0x0f ) + 0x50 );	// kh: spi says trailer is always "5" ?
 
-  _receive_length = packet_length;
+  if (_syncwordBytes != nullptr) {
+    _radio.openWritingPipe(_syncwordBytes);
+    _radio.openReadingPipe(1, _syncwordBytes);
+  }
 
-  _radio.openWritingPipe(_nrf_pipe);
-  _radio.openReadingPipe(1, _nrf_pipe);
+  _receive_length = packet_length;
 
   _radio.setChannel(2 + _channel);
-
   _radio.setPayloadSize( packet_length );
 
   return 0;
 }
 
-int PL1167_nRF24::setTrailerLength(uint8_t trailerLength)
-{ return 0; }
-/* kh- no thanks, I'll take care of that.
-   One could argue there is potential value to "defining" the trailer - such that
-   we can use those "values" for internal (repeateR?) functions since they are
-   ignored by the real PL1167..  But there is no value in _this_ implementation...
-*/
-
-int PL1167_nRF24::setCRC(bool crc)
-{
-  _crc = crc;
+int PL1167_nRF24::setSyncword(const uint8_t syncword[], size_t syncwordLength) {
+  _syncwordLength = syncwordLength;
+  _syncwordBytes = syncword;
   return recalc_parameters();
 }
 

From 7e9693cf8fd076d7bd8c9fb0be2f71b97b792333 Mon Sep 17 00:00:00 2001
From: Christopher Mullins <chris@sidoh.org>
Date: Sun, 30 Jun 2019 22:27:26 -0700
Subject: [PATCH 4/5] Misc cleanup

---
 lib/Radio/PL1167_nRF24.cpp | 30 +++++++++++++++++-------------
 lib/Radio/PL1167_nRF24.h   |  2 +-
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/lib/Radio/PL1167_nRF24.cpp b/lib/Radio/PL1167_nRF24.cpp
index c3ed1025..d2a4c72a 100644
--- a/lib/Radio/PL1167_nRF24.cpp
+++ b/lib/Radio/PL1167_nRF24.cpp
@@ -15,14 +15,12 @@
 #include <MiLightRadioConfig.h>
 
 static uint16_t calc_crc(uint8_t *data, size_t data_length);
-static uint8_t reverse_bits(uint8_t data);
 
 PL1167_nRF24::PL1167_nRF24(RF24 &radio)
   : _radio(radio)
 { }
 
-int PL1167_nRF24::open()
-{
+int PL1167_nRF24::open() {
   _radio.begin();
   _radio.setAutoAck(false);
   _radio.setDataRate(RF24_1MBPS);
@@ -62,14 +60,12 @@ int PL1167_nRF24::setSyncword(const uint8_t syncword[], size_t syncwordLength) {
   return recalc_parameters();
 }
 
-int PL1167_nRF24::setMaxPacketLength(uint8_t maxPacketLength)
-{
+int PL1167_nRF24::setMaxPacketLength(uint8_t maxPacketLength) {
   _maxPacketLength = maxPacketLength;
   return recalc_parameters();
 }
 
-int PL1167_nRF24::receive(uint8_t channel)
-{
+int PL1167_nRF24::receive(uint8_t channel) {
   if (channel != _channel) {
     _channel = channel;
     int retval = recalc_parameters();
@@ -123,8 +119,7 @@ int PL1167_nRF24::writeFIFO(const uint8_t data[], size_t data_length)
   return data_length;
 }
 
-int PL1167_nRF24::transmit(uint8_t channel)
-{
+int PL1167_nRF24::transmit(uint8_t channel) {
   if (channel != _channel) {
     _channel = channel;
     int retval = recalc_parameters();
@@ -157,9 +152,18 @@ int PL1167_nRF24::transmit(uint8_t channel)
   return 0;
 }
 
-
-int PL1167_nRF24::internal_receive()
-{
+/**
+ * The over-the-air packet structure sent by the PL1167 is as follows (lengths
+ * measured in bits)
+ *
+ * Preamble ( 8) | Syncword (32) | Trailer ( 4) | Packet Len ( 8) | Packet (...)
+ *
+ * Note that because the Trailer is 4 bits, the remaining data is not byte-aligned.
+ *
+ * Bit-order is reversed.
+ *
+ */
+int PL1167_nRF24::internal_receive() {
   uint8_t tmp[sizeof(_packet)];
   int outp = 0;
 
@@ -214,7 +218,7 @@ int PL1167_nRF24::internal_receive()
   _received = true;
 
 #ifdef DEBUG_PRINTF
-  printf("Successfully parsed packet of length %d\n", _packet_length);
+  Serial.printf_P(PSTR("Successfully parsed packet of length %d\n"), _packet_length);
 #endif
 
   return outp;
diff --git a/lib/Radio/PL1167_nRF24.h b/lib/Radio/PL1167_nRF24.h
index 408924f9..113d965a 100644
--- a/lib/Radio/PL1167_nRF24.h
+++ b/lib/Radio/PL1167_nRF24.h
@@ -33,7 +33,7 @@ class PL1167_nRF24 {
   private:
     RF24 &_radio;
 
-    uint16_t _syncword0 = 0, _syncword3 = 0;
+    const uint8_t* _syncwordBytes = nullptr;
     uint8_t _syncwordLength = 4;
     uint8_t _maxPacketLength = 8;
 

From e75ccd4a415593f9bcbc0d952f0c11eb2b7e5a33 Mon Sep 17 00:00:00 2001
From: Christopher Mullins <chris@sidoh.org>
Date: Sun, 30 Jun 2019 22:27:38 -0700
Subject: [PATCH 5/5] Misc cleanup

---
 lib/Radio/PL1167_nRF24.cpp | 77 ++++++++++++++------------------------
 lib/Radio/PL1167_nRF24.h   |  7 ++--
 2 files changed, 32 insertions(+), 52 deletions(-)

diff --git a/lib/Radio/PL1167_nRF24.cpp b/lib/Radio/PL1167_nRF24.cpp
index d2a4c72a..c0d1af6a 100644
--- a/lib/Radio/PL1167_nRF24.cpp
+++ b/lib/Radio/PL1167_nRF24.cpp
@@ -32,10 +32,11 @@ int PL1167_nRF24::open() {
   return recalc_parameters();
 }
 
-int PL1167_nRF24::recalc_parameters()
-{
+int PL1167_nRF24::recalc_parameters() {
+  int nrf_address_length = _syncwordLength;
+
+  // +2 for CRC
   int packet_length = _maxPacketLength + 2;
-  int nrf_address_pos = _syncwordLength;
 
   if (packet_length > sizeof(_packet) || nrf_address_length < 3) {
     return -1;
@@ -133,16 +134,16 @@ int PL1167_nRF24::transmit(uint8_t channel) {
   uint8_t tmp[sizeof(_packet)];
   int outp=0;
 
-  uint16_t crc;
-  if (_crc) {
-    crc = calc_crc(_packet, _packet_length);
-  }
+  uint16_t crc = calc_crc(_packet, _packet_length);
 
-  for (int inp = 0; inp < _packet_length + (_crc ? 2 : 0) + 1; inp++) {
+  // +1 for packet length
+  // +2 for crc
+  // = 3
+  for (int inp = 0; inp < _packet_length + 3; inp++) {
     if (inp < _packet_length) {
-      tmp[outp++] = reverse_bits(_packet[inp]);}
-    else if (_crc && inp < _packet_length + 2) {
-      tmp[outp++] = reverse_bits((crc >> ( (inp - _packet_length) * 8)) & 0xff);
+      tmp[outp++] = reverseBits(_packet[inp]);}
+    else if (inp < _packet_length + 2) {
+      tmp[outp++] = reverseBits((crc >> ( (inp - _packet_length) * 8)) & 0xff);
     }
   }
 
@@ -172,45 +173,35 @@ int PL1167_nRF24::internal_receive() {
   // HACK HACK HACK: Reset radio
   open();
 
-#ifdef DEBUG_PRINTF
-  printf("Packet received: ");
-  for (int i = 0; i < _receive_length; i++) {
-    printf("%02X", tmp[i]);
-  }
-  printf("\n");
-#endif
-
   for (int inp = 0; inp < _receive_length; inp++) {
-      tmp[outp++] = reverse_bits(tmp[inp]);
+    tmp[outp++] = reverseBits(tmp[inp]);
   }
 
-
 #ifdef DEBUG_PRINTF
-  printf("Packet transformed: ");
+  Serial.printf_P(PSTR("Packet received (%d bytes): "), outp);
   for (int i = 0; i < outp; i++) {
-    printf("%02X", tmp[i]);
+    Serial.printf_P(PSTR("%02X "), tmp[i]);
   }
-  printf("\n");
+  Serial.print(F("\n"));
 #endif
 
-
-  if (_crc) {
-    if (outp < 2) {
+  if (outp < 2) {
 #ifdef DEBUG_PRINTF
-  printf("Failed CRC: outp < 2\n");
+    Serial.println(F("Failed CRC: outp < 2"));
 #endif
-      return 0;
-    }
-    uint16_t crc = calc_crc(tmp, outp - 2);
-    if ( ((crc & 0xff) != tmp[outp - 2]) || (((crc >> 8) & 0xff) != tmp[outp - 1]) ) {
+    return 0;
+  }
+
+  uint16_t crc = calc_crc(tmp, outp - 2);
+  uint16_t recvCrc = (tmp[outp - 1] << 8) | tmp[outp - 2];
+
+  if ( crc != recvCrc ) {
 #ifdef DEBUG_PRINTF
-  uint16_t recv_crc = ((tmp[outp - 2] & 0xFF) << 8) | (tmp[outp - 1] & 0xFF);
-  printf("Failed CRC: expected %d, got %d\n", crc, recv_crc);
+    Serial.printf_P(PSTR("Failed CRC: expected %04X, got %04X"), crc, recvCrc);
 #endif
-      return 0;
-    }
-    outp -= 2;
+    return 0;
   }
+  outp -= 2;
 
   memcpy(_packet, tmp, outp);
 
@@ -240,14 +231,4 @@ static uint16_t calc_crc(uint8_t *data, size_t data_length) {
     }
   }
   return state;
-}
-
-static uint8_t reverse_bits(uint8_t data) {
-  uint8_t result = 0;
-  for (int i = 0; i < 8; i++) {
-    result <<= 1;
-    result |= data & 1;
-    data >>= 1;
-  }
-  return result;
-}
+}
\ No newline at end of file
diff --git a/lib/Radio/PL1167_nRF24.h b/lib/Radio/PL1167_nRF24.h
index 113d965a..b38dde78 100644
--- a/lib/Radio/PL1167_nRF24.h
+++ b/lib/Radio/PL1167_nRF24.h
@@ -20,11 +20,10 @@ class PL1167_nRF24 {
   public:
     PL1167_nRF24(RF24& radio);
     int open();
-    int setPreambleLength(uint8_t preambleLength);
-    int setSyncword(uint16_t syncword0, uint16_t syncword3);
-    int setTrailerLength(uint8_t trailerLength);
-    int setCRC(bool crc);
+
+    int setSyncword(const uint8_t syncword[], size_t syncwordLength);
     int setMaxPacketLength(uint8_t maxPacketLength);
+
     int writeFIFO(const uint8_t data[], size_t data_length);
     int transmit(uint8_t channel);
     int receive(uint8_t channel);