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

Native braille display drivers: provide a generic way to handle acknowledgement packets. #7721

Closed
leonardder opened this Issue Nov 2, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@leonardder
Collaborator

leonardder commented Nov 2, 2017

Issue summary

For several braille displays, when sending a packet, the display acknowledges the receipt of a packet. This is a mechanism to make sure that a packet has been delivered to a braille display in a correct way. Several displays have such a feature, including Handy Tech, Eurobraille and Freedom Scientific displays.

Proposal

Since there are multiple drivers with the need of proper ACK handling, I propose creating a generic system for this.

  1. Add three extra properties to the braille.BrailleDisplayDriver class.
    #: Whether displays for this driver return acknowledgements for sent packets.
    #: L{_handleAck} should be called when an ACK is received.
    #: Note that thread safety is required for the generic implementation to function properly.
    #: If a display is not thread safe, a driver should manually implement ACK processing.
    #: @type: bool
    receivesAckPackets = False
    #: Whether this driver is awaiting an Ack for a connected display.
    #: This is set to C{True} after displaying cells when L{receivesAckPackets} is True,
    #: and set to C{False} by L{_handleAck} or when C{timeout} has elapsed.
    awaitingAck = False
    #: Maximum timeout to use for communication with a device (in seconds).
    #: This can be used for serial connections.
    #: Furthermore, it is used by L{braille._BgThread} to stop waiting for missed acknowledgement packets.
    #: @type: float
    timeout = 0.2
    

Note that we can also put awaitingAck on the background thread.
2. in BrailleHandler._writeCells, check for not self.display.awaitingAck, and only queue the executor if we aren't waiting for an ACK and there is no queued write.
3. The executor should set awaitingAck to True whenever a succesful write has been made. Furthermore, it should start a waitable timer that, if not canceled by BrailleDisplayDriver._handleAck, logs the fact that an ACK has been missed, resets awaitingAck to False and queues the executor APC to the background thread.
4. BrailleDisplayDriver._handleAck should set awaitingAck to False and queue the executor APC to the background thread.

Thoughts from @jcsteh, @michaelDCurran and @bramd are appreciated.

@jcsteh

This comment has been minimized.

Contributor

jcsteh commented Nov 3, 2017

@leonardder

This comment has been minimized.

Collaborator

leonardder commented Nov 3, 2017

@jcsteh commented on 3 Nov 2017, 05:35 CET:

  1. You suggest that both the executor and _handleAck should "queue the
    executor APC to the background thread". These functions (the executor and
    _handleAck) will be running in the background thread, right? So we're
    actually queuing to the same thread we're in. There's no problem with that;
    I just want to be super clear.

That is true, but I was a bit reluctant to call a PAPCFUNC like a regular python function, so that's why I decided to queue here.

  1. I assume awaitingAck should never be touched by braille display drivers
    themselves; i.e. it is just used by core for managing state? If so, perhaps
    it should be underscore prefixed and the docstring amended to reflect that
    this is for internal use by core code only, not the driver.

That is true, I will do that.

@bramd

This comment has been minimized.

Contributor

bramd commented Nov 4, 2017

I just had a look at BRLTTY's code to see how they handle this.

They also abstracted the acknowledgement handling from the drivers into core. Each driver structure that uses this has the following attributes to keep state:

  • Alarm: set when waiting for an ACK/NAK, seems to be a timer that is set to missing.timeout
  • Messages: a queue of braille messages (see below)
  • Missing.count: number of missed ACKs, gets reset to 0 when an ACK is received
  • Missing.timeout: timeout to wait for an ACK

A braille message is a structure containing a packet and some metadata about it. Most importantly, every message gets a type. Usually, these types seem to follow the packet types (first byte of a packet in most protocols). If a packet needs to be send and there is already a queue in messages, the previous item in the queue with the same type is replaced. This is like what we do for braille cells, but more generic. This generic approach might com in handy when we are sending more messages to displays, such as dot pressure and a user is quickly changing settings.

If an ACK is received, or the timeout has been reached, the next packet from the queue is sent. Every time the timeout is reached, the missing counter is increased. If a max missing number of ACKs is reached, the driver goes into error state. As mentioned earlier, the missing counter is reset when an ACK is received.

Lessons we might learn from this:

  1. Make the ACK handling generic and not just for braille cells that should be written. Create a queue of packets and replace new packets that come in based on type
  2. Count missing ACKs and set an upper limit to this that should cause the driver to fail
@leonardder

This comment has been minimized.

Collaborator

leonardder commented Nov 4, 2017

Make the ACK handling generic and not just for braille cells that should be written. Create a queue of packets and replace new packets that come in based on type.

I agree this would be handy. Note that queueing messages to the background thread now also only applies to braille cells, so that's why I suggested building upon this system. If we make ACK handling more generic, we should make queueing to the thread more generic as well.

@jcsteh

This comment has been minimized.

Contributor

jcsteh commented Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment