Merge pull request #483 from sidoh/bugfix/rgb_listens

Bugfix: RGB listens with milight remote

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
 };

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 @@ public:
     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;

lib/Radio/NRF24MiLightRadio.cpp

@@ -35,22 +35,7 @@ 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);
+  int retval = _pl1167.setSyncword(_config.syncwordBytes, MiLightRadioConfig::SYNCWORD_LENGTH);
   if (retval < 0) {
     return retval;
   }

lib/Radio/PL1167_nRF24.cpp

@@ -11,89 +11,62 @@
  */
 
 #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);
 
 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);
   _radio.disableCRC();
 
-  _syncwordLength = 5;
+  _syncwordLength = MiLightRadioConfig::SYNCWORD_LENGTH;
   _radio.setAddressWidth(_syncwordLength);
 
   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 (_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::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)
-{ 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();
 }
 
-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();
@@ -147,8 +120,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();
@@ -162,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);
     }
   }
 
@@ -181,9 +153,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;
 
@@ -192,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);
 
@@ -238,7 +209,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;
@@ -260,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;
-}
+}

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);
@@ -33,11 +32,8 @@ class PL1167_nRF24 {
   private:
     RF24 &_radio;
 
-    bool _crc;
-    uint8_t _preambleLength = 1;
-    uint16_t _syncword0 = 0, _syncword3 = 0;
+    const uint8_t* _syncwordBytes = nullptr;
     uint8_t _syncwordLength = 4;
-    uint8_t _trailerLength = 4;
     uint8_t _maxPacketLength = 8;
 
     uint8_t _channel = 0;

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;
+}

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);