Merge pull request #313 from sidoh/312_fix_state-bug

Fix #312 -- multiple commands in same JSON would cause commands after 1st to get blank state

lib/MiLight/MiLightClient.cpp

@@ -33,6 +33,14 @@ void MiLightClient::begin() {
   }
 
   switchRadio(static_cast<size_t>(0));
+
+  // Little gross to do this here as it's relying on global state.  A better alternative
+  // would be to statically construct remote config factories which take in a stateStore 
+  // and settings pointer.  The objects could then be initialized by calling the factory
+  // in main.
+  for (size_t i = 0; i < MiLightRemoteConfig::NUM_REMOTES; i++) {
+    MiLightRemoteConfig::ALL_REMOTES[i]->packetFormatter->initialize(stateStore, settings);
+  }
 }
 
 void MiLightClient::setHeld(bool held) {
@@ -78,7 +86,7 @@ void MiLightClient::prepare(const MiLightRemoteConfig* config,
   this->currentRemote = config;
 
   if (deviceId >= 0 && groupId >= 0) {
-    currentRemote->packetFormatter->prepare(deviceId, groupId, stateStore, settings);
+    currentRemote->packetFormatter->prepare(deviceId, groupId);
   }
 }
 

lib/MiLight/PacketFormatter.cpp

@@ -28,6 +28,11 @@ PacketFormatter::PacketFormatter(const size_t packetLength, const size_t maxPack
   packetStream.packetLength = packetLength;
 }
 
+void PacketFormatter::initialize(GroupStateStore* stateStore, const Settings* settings) {
+  this->stateStore = stateStore;
+  this->settings = settings;
+}
+
 bool PacketFormatter::canHandle(const uint8_t *packet, const size_t len) {
   return len == packetLength;
 }
@@ -116,11 +121,9 @@ void PacketFormatter::valueByStepFunction(StepFunction increase, StepFunction de
   }
 }
 
-void PacketFormatter::prepare(uint16_t deviceId, uint8_t groupId, GroupStateStore* stateStore, const Settings* settings) {
+void PacketFormatter::prepare(uint16_t deviceId, uint8_t groupId) {
   this->deviceId = deviceId;
   this->groupId = groupId;
-  this->stateStore = stateStore;
-  this->settings = settings;
   reset();
 }
 

lib/MiLight/PacketFormatter.h

@@ -31,6 +31,13 @@ class PacketFormatter {
 public:
   PacketFormatter(const size_t packetLength, const size_t maxPackets = 1);
 
+  // Ideally these would be constructor parameters.  We could accomplish this by
+  // wrapping PacketFormaters in a factory, as Settings and StateStore are not
+  // available at construction time.
+  //
+  // For now, just rely on the user calling this method.
+  void initialize(GroupStateStore* stateStore, const Settings* settings);
+
   typedef void (PacketFormatter::*StepFunction)();
 
   virtual bool canHandle(const uint8_t* packet, const size_t len);
@@ -72,7 +79,7 @@ public:
   virtual void reset();
 
   virtual PacketStream& buildPackets();
-  virtual void prepare(uint16_t deviceId, uint8_t groupId, GroupStateStore* stateStore, const Settings* settings);
+  virtual void prepare(uint16_t deviceId, uint8_t groupId);
   virtual void format(uint8_t const* packet, char* buffer);
 
   virtual BulbId parsePacket(const uint8_t* packet, JsonObject& result);

src/main.cpp

@@ -90,15 +90,6 @@ void onPacketSentHandler(uint8_t* packet, const MiLightRemoteConfig& config) {
   StaticJsonBuffer<200> buffer;
   JsonObject& result = buffer.createObject();
 
-  // This is gross.  But if a packet is received for a remote type before one is
-  // sent, prepare() won't have been called, meaning stateStore and settings will
-  // not have been initialized.  Both of these things should either be passed in
-  // as constructor parameters, or as prameters to the methods that require them.
-  // 
-  // But for now, just hackily call prepare.  At least fixes a bug.  The deviceId
-  // and groupId won't matter.
-  config.packetFormatter->prepare(0, 0, stateStore, &settings);
-
   BulbId bulbId = config.packetFormatter->parsePacket(packet, result);
 
   // set LED mode for a packet movement