corneliusmunz / legoino

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

Lego Boost issues/feedback found on migrating to legoino V1.01 #33

Closed hrgraf closed 3 years ago

hrgraf commented 3 years ago

Hi Cornelius, first of all thank you for this cool and active library. Congratulations for reaching a version 1, quite a milestone!

I was not aware that the API is that much under development, so I had to completely rework my Lego Boost example (AtomMatrixVernie in https://github.com/hrgraf/M5), which I only have started 1 month ago...

But organizing the library for more callbacks makes sense to me and seems to be more adapted to the BLE world. Plus: new features such as Hub emulation sound very interesting.

Now about the issues that I have found during migration: (tested with Lego Boost, my only powered up device)

1) a lot of data type casting in the new API, e.g. enum to byte, even in code that is not time critical e.g. for port in activatePortDevice()

2) don't understand the issue with delay() required for activatePortDevice() but even doing so, I get no HW/FW versions reported

3) distance sensor unit does not seem to be cm, rather mm

4) color sensor is reporting undefined color For me, reporting an undefined color is not an error, but a normal situation, that should be handled in the application, and not lead to error messages: [E][Lpf2Hub.cpp:424] parseColor(): undefined color (255) E.g. the example MoveHubColorSensor.ino crashes on line 34, because it does not check the color range For color range checking, there is no constant defined in your API I suggest the following (which I did in my fork of legoino). If you want, I can raise a pull request, but it is that simple:

--- a/src/Lpf2HubConst.h
+++ b/src/Lpf2HubConst.h
@@ -208,10 +208,11 @@ typedef enum Color
   ORANGE = 8,
   RED = 9,
   WHITE = 10,
+  NUM_COLOR,
   NONE = 255
 };

5) few and late callbacks e.g. on distance or tilt sensor (This might be an issue of the Lego boost hub itself) I found that I get max. 15 updates (callback) in total per second, while I tilt the hub etc. The distance sensor has some lag, sometimes reports distance updates only after 1s.

6) As you are using an AtomMatrix ESP32 device yourself, you could try out my AtomMatrixVernie example in https://github.com/hrgraf/M5). It is a nice demonstration of your legoino library and you could add it to your examples.

Cheers, Hans-Rudolf

corneliusmunz commented 3 years ago

Hi Hans-Rudolf (@hrgraf)! Thank you very much for your detailed description of your issues which you had during the migration to the version 1.0.1. I highly appreciate any feedback especially when it is so profound! Sorry for your efforts to be forced to migrate to the 1.0.0 version. It is really hard as a maintainer of such a library to decide if a restructuring is ok or should not be done. I decided to change it now because there were many "wrong" assumptions and decisions done in the former versions and i have learned a lot.

  1. a lot of data type casting in the new API, e.g. enum to byte, even in code that is not time critical e.g. for port in activatePortDevice()

Thats correct and i try to make it more consistent in the future.

  1. don't understand the issue with delay() required for activatePortDevice() but even doing so, I get no HW/FW versions reported

It is a little bit difficult for me to understand the correct timings of the PoweredUp devices. Some devices work without delays. some skip that message if you send it too early or too fast. One finding is that you should only send the activatePortDevice command if the hub has recognised the device on that port an reported that the device is connected. I have implemented the check if a device is connected on a port in the TrainColor.ino sketch:

 byte portForDevice = myHub.getPortForDeviceType((byte)DeviceType::COLOR_DISTANCE_SENSOR);
    Serial.println(portForDevice, DEC);
    if (portForDevice != 255)
    {
    ...
    }
  1. distance sensor unit does not seem to be cm, rather mm

Thats correct. I will change that in the comment

  1. color sensor is reporting undefined color

Thats also a good catch. The function itself works fine but if you have enabled logging, the log level is definitely wrong. The improvement of the range check is a good idea. I am struggling a little bit with the 255 value of undefined color. If the COLOR_STRING array is used, the index 255 is not valid. Maybe you have an idea how to handle this.

  1. few and late callbacks e.g. on distance or tilt sensor (This might be an issue of the Lego boost hub itself) I found that I get max. 15 updates (callback) in total per second, while I tilt the hub etc. The distance sensor has some lag, sometimes reports distance updates only after 1s.s

I think that is not an issue of the library. The only thing i can do is to change the mode of the color/distance sensor that only color or distance is reported and not both. I will try to check that.

  1. As you are using an AtomMatrix ESP32 device yourself, you could try out my AtomMatrixVernie example in https://github.com/hrgraf/M5). It is a nice demonstration of your legoino library and you could add it to your examples.

Very cool example sketch. I would like to link that example in the documentation. Then you can maintain it and it is clear who has done the contribution

Cheers, Cornelius

And btw. you are always open to do a Pull Request. Then your contribution will be even more visible

hrgraf commented 3 years ago

Hi @corneliusmunz,

thank you for the answers.

For the color range issue: I would add NUM_COLOR (equal 11) to the Color enum, so the user/library can check, if the detected color is valid. Like I do in my example/fork.

BTW: Do you have an idea, why I don't get HW/FW versions reported?

corneliusmunz commented 3 years ago

Hi Hans-Rudolf (@hrgraf)! I think i found the issue related to the HW/FW version. I also never got a version value back but the version is nothing which will be triggered with the EnableUpdates flag https://lego.github.io/lego-ble-wireless-protocol-docs/index.html#hub-property-operation You have to request explicitly an Update with the value 0x05and you can do this in the legoino library with the following command:

myMoveHub.requestHubPropertyUpdate(HubPropertyReference::FW_VERSION, hubPropertyChangeCallback);
myMoveHub.requestHubPropertyUpdate(HubPropertyReference::HW_VERSION, hubPropertyChangeCallback);

I have tried it out with an adapted MoveHubDeviceInfo.ino sketch and now i got one single HW and FW version :smile:

Here you can find the adapted sketch (just rename it to .ino and run it) Hope it works for you too: MoveHubDeviceInfo.txt

Cheers, Cornelius

corneliusmunz commented 3 years ago

@hrgraf For the battery type value it is the same behaviour. It needs a explicit request of the value. I think this is the case for all properties which does not really change over one "power up lifecycle"

hrgraf commented 3 years ago

@corneliusmunz

thank you, requestHubPropertyUpdate() is working, I get HW/FW version reports

corneliusmunz commented 3 years ago

fixed with release https://github.com/corneliusmunz/legoino/releases/tag/1.0.2

Mattze0815 commented 3 years ago

Confirm getting battery events now.