corneliusmunz / legoino

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

Include 'Lpf2Hub* hub' parameter in HubPropertyChangeCallback ? #24

Closed fvanroie closed 4 years ago

fvanroie commented 4 years ago

I'm updating my code to test the new capabilities in the feature/add_hub_emulation branch. In my code I have multiple tasks connecting to multiple PU hubs. I'm a bit stuck with the new callback functions.

It seems each callback can only be used with one specific hub e.g. global myMoveHub, since the callback has no way of knowing for which hub it is receiving the update. Would it make sense to include a Lpf2Hub* this property, or something to distinguish the affected Hub?

The callback might be;

// callback function to handle updates of hub properties
void hubPropertyChangeCallback(Lpf2Hub* hub, HubPropertyReference hubProperty, uint8_t *pData)
{
  Serial.print("HubProperty: ");
  Serial.println((byte)hubProperty, HEX);

  if (hubProperty == HubPropertyReference::RSSI)
  {
    Serial.print("RSSI: ");
    Serial.println(hub->parseRssi(pData), DEC);  // use this-> instead of a global myMoveHub
    return;
  }
}

I want to avoid having to write several callbacks, one for each of hub in the network. What do you think?

corneliusmunz commented 4 years ago

Hi @fvanroie Thanks for raising this issue! Another User (@M9Lab) has asked the same issue in the gitter chat: https://gitter.im/legoinochat/community?at=5f773b54fcce3e6c18ea2cbd I am thinking about how to handle this and including this in the v1.0.0 version. I think that is a common requirement of all users who will have multiple hubs connected.

corneliusmunz commented 4 years ago

@fvanroie I have restructured the whole library in that feature branch which your are evaluating. So be aware that some Breaking changes are there. And it could happen that the code in the feature branch does not work πŸ˜‰ Here you can find a draft version of the migration documentation which will be provided by that new release with major changes: https://github.com/corneliusmunz/legoino/blob/feature/add_hub_emulation/doc/MIGRATION.md

fvanroie commented 4 years ago

Thanks for the update. I am aware about the breaking changes and the migration steps needed. I have read that documentation, which is very helpful indeed! I am working on a copy of my project to test and make use of the new (beta) features. Very exciting stuff!

What I currently did is make these manual changes in Lpf2Hub.h:

typedef void (*HubPropertyChangeCallback)(void *hub, HubPropertyReference hubProperty, uint8_t *pData);

and Lpf2Hub.cpp

void Lpf2Hub::parseDeviceInfo(uint8_t *pData)
{
    if (_hubPropertyChangeCallback != nullptr)
    {
        _hubPropertyChangeCallback(this, (HubPropertyReference)pData[3], pData);
        return;
    }

Now my hubPropertyChangeCallback function looks like this:

void hubPropertyChangeCallback(void *hub, HubPropertyReference hubProperty, uint8_t *pData)
{
    Lpf2Hub *myHub = (Lpf2Hub *)hub;

    Serial.print("HubAddress: ");
    Serial.println(myHub->getHubAddress().toString().c_str());

    Serial.print("HubProperty: ");
    Serial.println((byte)hubProperty, HEX);

    if (hubProperty == HubPropertyReference::ADVERTISING_NAME)
    {
        Serial.print("HubName: ");
        Serial.println(myHub->parseHubAdvertisingName(pData).c_str());
        return;
    }
}

And it seems to work. πŸ˜ƒ

Scanning task1 on core 0
Hub connected + Release.
System is initialized
HubAddress: 90:84:2b:1c:be:3e
HubProperty: 2
Button: 0
HubAddress: 90:84:2b:1c:be:3e
HubProperty: 6
BatteryLevel: 100
corneliusmunz commented 4 years ago

@fvanroie I think your changes should work for all use cases and i think this will be a generic implementation πŸ‘ You can ask for the Name, or the Address, or the type of the hub and can then do things dependent on hub type or directly call functions on that hub reference. If you would like, you can do a Pull Request on the feature branch and then your contribution will be visible πŸ‘ Or if you don't want this i will add your proposed changes by my own. So just give me a short feedback!

By the way: I am very curious for which setup you are using the library? Do you have a huge train setup? Do you have any other missing features?

fvanroie commented 4 years ago

Hi @corneliusmunz. I will create a PR, no problem.

I am currently testing with 5 PoweredUp Hubs and 5 Remote's. The idea is to power a large train layout, controlled by a train automation software, like RocRail. I'm testing Legoino with MQTT on the ESP32 to control the trains. Later I want to add track sensors and switch motors via GPIO.

fvanroie commented 4 years ago

The changes are included in PR #25. It also includes some minor updates to the Migration.md docs that you may want to consider.

corneliusmunz commented 4 years ago

Fixed with Release 1.0.0 https://github.com/corneliusmunz/legoino/releases/tag/1.0.0