corneliusmunz / legoino

Arduino Library for controlling Powered UP and Boost controllers
MIT License
257 stars 34 forks source link

playSound and setLedColor very unreliable #44

Open theohm opened 3 years ago

theohm commented 3 years ago

Libraries: Legoino 1.0.2, NimBLE-Arduino 1.0.2 ESP32 board: AZ-Delivery ESP32 NodeMCU V2 Lego model: Duplo Loco from the 10874 set

Cornelius, big thanks for your Legoino library!

When I use Legoino, the methods setBasicMotorSpeed and stopBasicMotor both work perfectly every time. On the other hand, playSound and setLedColor are very unreliable and only do their job in about one out of three times called. Any ideas what I do wrong? Is there a known bug? Maybe in conjunction with certain ESP32 boards?

Attaching an .ino file renamed to .txt: Simplified.ino

corneliusmunz commented 3 years ago

Hi @theohm ! Thanks for your feedback and thanks for opening your first issue 👏 ! I highly appreciate any feedback on the library 👍

I also spotted some unreliable behaviour in playing sounds for the duplo train hub. I will have a more deeper look on that issue. I will try it out with another library (.net and node) and see if the behaviour is better. Your *.ino sketch looks quite ok.

corneliusmunz commented 3 years ago

@theohm Have you tried the example *.ino sketch which i have send to you? Was it better with removing the delays out of your sketch?

theohm commented 3 years ago

@corneliusmunz Sorry, I missed your message with the new sketch. How did you send it?

Removing the delay from my *.ino sketch would also disable debouncing of the buttons, so I did not completely remove it, but reduce it from 200 to 10. That did not change the behaviour.

Then I saw that there is a new Legoino version avaiblable in the Arduino IDE. So i updated to Legoino 1.0.2, which improved the reliability a bit. Now, only approximately every 3rd sound command is lost. After returning to a delay of 200, not even every 2nd sound command is executed, so the delay seems to affect the reliability a little bit. Reliability at a delay of 500 does not significantly differ from that at a delay of 200.

corneliusmunz commented 3 years ago

@theohm I have send you a mail with the sketch... maybe it is in the spam folder

corneliusmunz commented 3 years ago

Hi @theohm i have had a look on your sketch and added the following sketch as example with the JC_Button library (just rename it to *.ino): DuploHornButtonTest.txt With this sketch almost every event will trigger a sound. Sometimes it does not work but i think this only happens if a sound already is playing on the hub and the hub will cancel duplicate events on the sound module and does not allow to trigger a sound while another sound output is still running.

manuelm commented 2 years ago

Hi there.

I recently build a remote control for my kid and noticed the same. Sound and color commands are unreliable and only work at about 2 out of 3 commands. However motor commands work all the time. So I looked at the bluetooth command bytes and noticed that motor commands are one bluetooth command (a single 0x81 port output command) vs sound/color are two (an additional 0x41 port input format setup beforehand). I then split the two commands into two functions and only sent the port input format after a successful connection to the hub. And now sound + color works 100% reliable.

I'm not sure why the current code always sends both commands. Imho it's not necessary.

So I basically did this:


class MyHub
  : public Lpf2Hub
{
public:
  void activateBaseSpeaker()
  {
    byte setSoundMode[8] = { 0x41, 0x01, 0x01, 0x01, 0x00, 0x00, 0x00, 0x01 };
    WriteValue(setSoundMode, 8);
  }

  void playSound(byte sound)
  {
    byte playSound[6] = { 0x81, 0x01, 0x11, 0x51, 0x01, sound };
    WriteValue(playSound, 6);
  }

  void activateRgbLight()
  {
    byte port = getPortForDeviceType((byte)DeviceType::HUB_LED);
    byte setColorMode[8] = { 0x41, port, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00 };
    WriteValue(setColorMode, 8);
  }

  void setLedColor(Color color)
  {
    byte port = getPortForDeviceType((byte)DeviceType::HUB_LED);
    byte setColor[6] = { 0x81, port, 0x11, 0x51, 0x00, color };
    Lpf2Hub::WriteValue(setColor, 6);
  }
}

void loop()
{
  if (myHub.isConnecting())
  {
    myHub.connectHub();
    if (myHub.isConnected())
    {
      [...]
      myHub.activateRgbLight();
      delay(200);
      myHub.activateBaseSpeaker();
      delay(200);
      [...]
    }
  }

  if (myHub.isConnected())
  {
    [...]
  }
corneliusmunz commented 2 years ago

@manuelm Many thanks for that finding! 👏 👍 Very good question why i have implemented this in that way. I think i have used the implementation of https://github.com/nathankellenicki/node-poweredup/blob/master/src/devices/duplotrainbasespeaker.ts as a basis but here the state of the activated mode is checked and if the mode is equal to the current mode, no message is sent out. So maybe i will change your implementation a littlebit and try to store the state. Then i will only send the message if the state differs to the current state.

manuelm commented 2 years ago

Sure. Just make sure to send both commands with a short delay. Wasn't reliable otherwise.

theohm commented 2 years ago

Thanks @manuelm for your solution! @corneliusmunz: Will you eventually patch Manuels code into the official release?

corneliusmunz commented 2 years ago

@theohm I have already a local fix which is very similar to the solution from @manuelm I have to do some additional testing but I will definitely release it hopefully in the next days

yschroeder commented 7 months ago

I added the above implementation to my sketch and can confirm it is working well!

Would be great if this would be released as a bugfix release.

Thank you for the great library! I am currently building a remote control for my son's train, so right now I am having fun building it and then he will be having fun using it 😃