Add a baton to the handler callback

[eric-wieser]

Note that unless we want to break backwards compatibility, we need to support functions with and without batons, hence the union here.

Why this is useful:

class MyClassWithAHandler;

void _handle(const uint8_t* buffer, size_t size, void* baton) {
   auto this_ = static_cast<MyClassWithAHandler*>(baton);
   this_->handle(buffer, size);
}

class MyClassWithAHandler {
public:
    PacketHandler _handler;
    MyClassWithAHandler() {
        _handler.setPacketHandler(_handle, this);
    }
    void handle(const uint8_t* buffer, size_t size) {

    }
}

[bakercp]

Hi @eric-wieser thanks for this contribution it looks great so far (I still want to play with it a bit before a merge).

A few questions -- first would you be opposed to calling it a context rather than a baton? Based on other libraries I've used, I feel like calling it context in the callback function would be more familiar. I'm definitely up for discussion though -- maybe I'm just not up to date with the latest terminology.

Second, instead of keeping a separate boolean for hasBaton could we just set the baton to a nullptr by default and check for nullptr rather than using an additional bool?

Thanks again!

[eric-wieser]

I've barely used any libraries with this mechanism under either name, so I'll defer to your more informed judgement - context definitely sounds more descriptive, but under my tiny sample of proprietary code, baton was more common.

Could we just set the baton to a nullptr by default

If you do that, then the setPacketHandler(myfunc, nullptr) would trigger dangerous behaviour, since the wrong member of the union would be invoked.

[eric-wieser]

Is breaking compatibility out of the question? This is likely a once-per-project change of just adding void* to the end of the packethandler definition (assuming we allow the context to default to null)

[bakercp]

I've decided to take a different approach to keep the library simpler. Check out the addition of the const void* sender variable in the packet handler. Perhaps it could help solve this problem? Once C++11/C++14 is more widely supported by all Arduino variants, we can take on your request with std::function, etc. Thanks!

https://github.com/bakercp/PacketSerial/blob/master/examples/PacketSerialReverseEchoAdvanced/PacketSerialReverseEchoAdvanced.ino

Closing this for now.

[eric-wieser]

Why is the argument of type void * and not type PacketSerial *?

I've decided to take a different approach to keep the library simpler.

I don't think this is the right choice. Knowing the sender isn't useful - you want to know extra data that you want to store with the sender,

If all you pass is the sender, then you may as well have just attached separate functions. This:

packet1.setPacketHandler(&onPacketReceived);
packet2.setPacketHandler(&onPacketReceived);

void onPacketReceived(const void* sender, const uint8_t* buffer, size_t size) {
    if (sender == &packet1) {
        func1(...);
    }
    elif (sender == &packet2) {
        func2(...)
    }
}

is not more useful than

packet1.setPacketHandler(&func1);
packet2.setPacketHandler(&func2);

[eric-wieser]

Here's a simpler example of why a real baton/context is useful.

Suppose you have some serial connections that are supposed to emit the next letter of the alphabet after each incoming message. With your sender, you could write that as:

struct my_sender {
    char letter;
    PacketSerial serial;
};

my_sender s1;
my_sender s2;
int main() {
    s1.letter = 'a';
    s2.letter = 'm';

    s2.serial.setPacketHandler(&onPacketReceived);
    s1.serial.setPacketHandler(&onPacketReceived);
}

void onPacketReceived(const void* sender_raw, const uint8_t* buffer, size_t size) {
    my_sender *sender;
    if (sender_raw == &s1.serial) {
        sender = &s1;
    }
    else if (sender_raw == &s2.serial) {
        sender = &s2;
    }
    sender->serial.send(&sender->letter, 1);
    sender->letter++;
}

And with a full context

struct my_sender {
    char letter;
    PacketSerial serial;
};

int main() {
    // these don't need to be global any more!
    my_sender s1;
    my_sender s2;
    s1.letter = 'a';
    s2.letter = 'm';

    s2.serial.setPacketHandler(&onPacketReceived, &s2);
    s1.serial.setPacketHandler(&onPacketReceived, &s1);
}

void onPacketReceived(const void* context, const uint8_t* buffer, size_t size) {
    my_sender *sender  = context;
    /* No more ifs! */
    sender->serial.send(&sender->letter, 1);
    sender->letter++;
}

How about allowing any void* to be passed, but defaulting to using this like you currently do? That way, your example you link to would continue to work, but more complex cases would be supported too.

Once C++11/C++14 is more widely supported by all Arduino variants, we can take on your request with std::function

Note that if you pass a void*, then this library is already usable on platforms with std::function!

std::function<void (uint8_t*, size_t)> f = [](uint8_t* buffer, size_t size) {

};
serial.setPacketHandler(call_std_function, &f);

void call_std_function(const void* context, const uint8_t* buffer, size_t size) {
    std::function<void (uint8_t*, size_t)> *f = context;
    (*f)(buffer, size);
}

[bakercp]

Hey @eric-wieser I appreciate your persistence and I think I better understand your use-case now. I just added to the advance example. It should allow you to do what you want to using lambdas ... I think :) Give it a look:

https://github.com/bakercp/PacketSerial/commit/c4a9757cf8c66d4692a6591e96a6f9daf31e2008

[eric-wieser]

@bakercp: I'm back, sorry for forgetting about my persistence.

The problem with your approach is it does not let me use lambdas with captures like:

void main() {
    int num_packets = 0;
    myPacketSerial.begin(115200);
    myPacketSerial.setPacketHandler([&](const uint8_t* buffer, size_t size) {
         num_packets += 1;
    });
    while (true) {
        ...
    }
}

Batons / context arguments to callbacks are the primitive feature needed to build something more complex. If there were a baton argument, you could convert a lambda into a func, baton pair:

# note: no need for `std::function` because we pass `func` by reference.
template<typename F>
void setLambdaHandler(PacketSerial& serial, F& func) {
    myPacketSerial.setPacketHandler(
        // callback knows how to call the lambda
        [](const uint8_t* buffer, size_t size, void* baton){
            reinterpret_cast<F*>(baton)(buffer, size);
        },
        // baton is just the opaque lambda
        reinterpret_cast<void*>(&func)
   );
}

void main() {
    int num_packets = 0;
    myPacketSerial.begin(115200);
    auto func = [&](const uint8_t* buffer, size_t size) {
         num_packets += 1;
    };
    setLambdaHandler(myPacketSerial, func);
    while (true) {
        ...
    }
}