electric-sheep-co / arduino-redis

A Redis client library for Arduino.
http://arduino-redis.com
MIT License
54 stars 19 forks source link

parseType and startSubscribing are blocking #57

Closed holla2040 closed 1 year ago

holla2040 commented 1 year ago

The current architecture uses startSubscribing which blocks, hence main loop is never called. This is a serious limitation, the micro can't do anything else.

I've looked over the code startSubscribing blocks on subLoopRun which calls parseType that blocks on client.available()

I think it should work like this instead. subscriptionSetup msgCB and errCB are setup subscriptionLoop <<<< doesn't block

then users loop looks like this

void loop() { doThings(); subscriptionLoop(); }

I dug into startSubscribing and parseType to see if non-blocking was a simple addition. It is not (well, at least with my aged coding skills). There are statements in there I have no clue about, all the template stuff.

I'd pay for a non-blocking option to be implemented correctly. Otherwise, my blocking workaround is to call in startSubscribing in a FreeRTOS task on ESP32.

harmrochel commented 1 year ago

Hi, I had the same issue and hacked together a solution which is doing the job for me but might also not be the cleanest approach: https://github.com/electric-sheep-co/arduino-redis/compare/master...harmrochel:arduino-redis:subscribe-non-blocking I just converted startSubscribing into a new method called startSubscribingNonBlocking which takes a function as additional argument. That function is now invoked everytime the original startSubscribing/parseType would block. So you can just pass loop() to startSubscribingNonBlocking and it will be executed again while the redis stuff is handeled in between.

rpj commented 1 year ago

@harmrochel that is excellent work, well done indeed, thank you! I like the callback implementation choice you made on your fork and would be happy to integrate such a change into the library properly. Would you want to submit these changes as a PR to be merged into the library?

harmrochel commented 1 year ago

Hi @rpj, sure. I created a pull request: https://github.com/electric-sheep-co/arduino-redis/pull/58

holla2040 commented 1 year ago

@harmrochel I was just about to do something similar. I'll try yours out.

I did look over your code and have a question

In your approach, in parseTypeNonBlocking(), isn't typeChar = (RedisObject::Type)client.read(); blocking? maybe look at client.available() first.

std::shared_ptr<RedisObject> RedisObject::parseTypeNonBlocking(Client &client)
{
    if(client.connected() && !client.available()) {
        return nullptr;
    }

    RedisObject::Type typeChar = RedisObject::Type::NoType;
    if (!client.connected())
    {
        return std::shared_ptr<RedisObject>(new RedisInternalError(RedisInternalError::Disconnected));
    }

    typeChar = (RedisObject::Type)client.read();
    if(typeChar == -1 || typeChar == '\r' || typeChar == '\n'){
...
}
harmrochel commented 1 year ago

@holla2040 client.read() is already doing client.available() before it actually tries to receive data (see https://github.com/lstoll/arduino-libraries/blob/master/Ethernet/Client.cpp#L84)

holla2040 commented 1 year ago

Oh, that's great. Thanks for the quick reply.

holla2040 commented 1 year ago

@harmrochel verified your nonblocking code functions correctly.

auto subRv = redis.startSubscribingNonBlocking(msgCallback, loop, errorCallback); I can set blink with via a redis message in msgCallback. ... void loop() { if (blink) { digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN)); delay(50); } }

Thanks for the effort.

holla2040 commented 1 year ago

resolved in v2.5