Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pause Before Writing if Additional Hops Remaining #45

Closed
krkeegan opened this issue Mar 11, 2018 · 6 comments
Closed

Pause Before Writing if Additional Hops Remaining #45

krkeegan opened this issue Mar 11, 2018 · 6 comments

Comments

@krkeegan
Copy link
Collaborator

Another complex but important concept is:

  • For each incoming message look at the hops left flag
  • Pause before writing data to the modem until the necessary amount of time has elapsed for those hops to arrive.

This helps avoid a number of issues, particularly duplication issues.

I tried to implement this, but I hit a bit of a wall. The deduplication PR calculates the necessary expire time. Now I just need to tell the correct function to wait. Initially, I put this wait all the way down in the Serial class inside the write_to_link class. This worked perfectly, except the Protocol class thinks it has already sent the message and so has "enabled" the write_handler which may now inadvertently see and misinterpret a message coming in.

The better thing is to delay the send from the Protocol class. However, I don't see anything that would enable polling to catch an outgoing message that is waiting. The _poll() function in protocol only seems to be called about every 3 seconds.

On a related note, I have also seen instances of writing to the PLM too fast when using other programs. Even when the PLM is properly acknowledging all messages. Eventually it responds with a 0x15 message which is supposed to be interpreted as a slow down message. So we may want a generic throttle that prevents messages from being sent any more frequently than X time.

@krkeegan
Copy link
Collaborator Author

One solution I see is to use the _msg_written callback in the protocol. It could wait until called to enable the write_handler.

This however seems a bit fragile to me, because the _msg_written callback is only passed the Serial object and the bytearray of the message written. It would be better if it could actually pass back the message object, but of course it never has that object.

@TD22057
Copy link
Owner

TD22057 commented Mar 30, 2018

FYI the poll method is called every 3 sec because that's the time out in the network loop. It's more efficient to have a long time out to respond to socket inputs but MQTT required a periodic poll call to send keep alive messages which is why I added that method.

I think the correct approach is to add a timer system so that the Protocol class can say either "write this after X time" or maybe "call me back after time X" so it can do the write.

@TD22057
Copy link
Owner

TD22057 commented Apr 7, 2018

I was looking at those callbacks in Protocol.py and I think there is a subtle bug. _send_next_msg calls handler.sending_message(msg) which updates the expire time. But the message wasn't actually sent - that happens in _msg_written. One solution to that problem is to not pop the msg,handler tuple off the queue until it's written. Then pop it off in _msg_written and tell the handler to update the expire time.

That has the side benefit of making the msg available in _msg_written. But I'm not sure that helps with pausing between writes. One possible solution is to add a "next write time" compute in _process_msg() that sets a member variable based on the hop compute of the next time to write. Then in _send_next_msg(), check the time vs that time to decide to write or not. Since the write_handle update is moved to _msg_written(), then the _send_next_msg() can decide not to write and I think it will be ok. I'll give this a try and see what happens.

@TD22057
Copy link
Owner

TD22057 commented Apr 7, 2018

That last suggestion above won't work. I think the network link write() code has to be updated to have an optional after_time input. Then when the link tries to write the bytes, it can skip it if the time hasn't elapsed enough.

I think this will work but it might be quite inefficient because the network event loop will be continuously returning since there is data to write. It would be better to adjust the network poll/select call time out to wait until the right amount of time has elapsed. I'll do some testing and see what happens.

@TD22057
Copy link
Owner

TD22057 commented Apr 7, 2018

OK - Tests seem to work for this. I pushed a new branch "wait_to_write" with the delta's - please take a look when you have a chance.

To test this, I added a 10 sec delay to Protocol line 304 where it sets the input msg expire time as the next write time. Then I sent two on commands from the command line. The first one sends and then there are ton of "waiting to write" debug messages as the network loop repeatedly calls the Serial write callback which waits to write. Once 10 sec elapses, it does the write. As I said before, this isn't the most efficient way to to do this but I think it does work.

TD22057 added a commit that referenced this issue Jul 7, 2018
Wait for expire time before writing more messages.  Fixes issue #45
@TD22057
Copy link
Owner

TD22057 commented Jul 7, 2018

Since the branch is working for me, I've submitted the fix in PR #74 into the dev branch.

@TD22057 TD22057 closed this as completed Jul 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants