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

ANT+ messages lost - refactoring of ANT-loop required? #341

Closed
WouterJD opened this issue Dec 2, 2021 · 20 comments
Closed

ANT+ messages lost - refactoring of ANT-loop required? #341

WouterJD opened this issue Dec 2, 2021 · 20 comments
Assignees
Labels
enhancement New feature or request implemented

Comments

@WouterJD
Copy link
Owner

WouterJD commented Dec 2, 2021

At times it seeems that ANT-messages are not received, causing HRM-connection being lost or other unexplainable inconveniences.
The suggestion is done to refactor the ANT-loop, this issue is meant to

  • discuss the subject
  • get to a better implementation (if required)
  • or document why the current implementation is satisfactory

See also Steering - packet drops

@WouterJD WouterJD mentioned this issue Dec 2, 2021
@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

Let's check the main loop that starts here

This loop operates as follows:

while always
  read from Tacx Trainer

  update display, TCX, JSON

  calculate PedalStrokeAnalysis (PSA)

  every quarter second:
    prepare messages for HRM, PWR, SCS, FE-C
    send/receive messages to/from blue tooth devices

  the following occurs only if there are messages to be sent, so once per quarter second as well
    send/receive ANT+ messages
    handle all received ANT+ messages

  delay the loop, so that it will cycle at the required speed

The loop can operate on two speeds:

  • 250ms; so trainer and ANT are updated at a 4/second interval
  • 20ms; so trainer is read every 20ms (for PSA) and ANT is updated every quarter second

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

The ANT+ reading is done every 250ms into a 1000 character buffer (for details click here).
One read-operation can provide multiple packets, FortiusAnt expects only entire ANT+packets to be delivered, otherwise error-messages are given:

  • Dongle.Read() too much data from .read()
  • "%s characters skipped "
  • "error: checksum incorrect"
  • "error: message exceeds buffer length"

Since these messages never (or perhaps more honestly: rarely) appear:

  • The USB driver never provides more data than 900 bytes (where 1000 are available)
  • Always provides entire ANT+packets

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

The question now is whether packets are lost because FortiusAnt reads data from the USB-driver as described.
Also: would a sparate thread that is dedicated to read data from the USB-driver resolve the issue?

Currently, I think that we should be aware that we read data from the USB-device driver that in it's turn reads data from the USB-dongle. I would expect the USB-device driver to have it's own process, handling the USB-dongle and provides data to FortiusAnt when we do the read-command.

In line with that thought - but I do not know device driver internals, let alone I ever created such - I would not expect a separate program-thread to help in resolving the lost packet issue.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

Issues that have been observed:

  • When using a USB-hub, packets are lost. This was resolved by using another USB-port (or not a hub at all). This observation indicates that timing between the (windows)computer(-device-driver) and the ANT-dongle is important.
  • FortiusAnt' ANT-dongle and Zwift'ANT-dongle communicate better with a clear sky; there should not be computers in between. Two dongles in the same computer work good when sitting next to each other.
  • The connection with the HRM heartrate belt is lost frequently, not only by FortiusAnt but also Zwift and Trainer Road, regardless whether one or two PC's are used. The distance from HRM to the ANT-dongles seems important. At the time this occurs, a Garmin watch does still show the HR.
    Note 1: although ANT+ specifications state that 30m distance between devices is allowed/possible, the practical distance seems a lot shorter (1.5 m).
    Note 2: the expected usage for HRM is at chest-to-arm distance and for steering device is at steering-to-trainer distance; where expected distance for trainer-to-computer is expected to be larger. Note that the trainer has a mains connection, where a HRM is battery driven, possibly affecting antenna transmission power.

I will do a test with a USB-hub, bringing the dongles closer to the bike (and myself and HRM) to check whether this affects the HRM-issue. Perhaps this also gives usefull information for Black track steering and ANT-based trainers.

So far my input on the ANT+ messages lost issues for now :-)

@WouterJD WouterJD added enhancement New feature or request under investigation Being studied for implementation in next version labels Dec 2, 2021
@switchabl
Copy link
Contributor

switchabl commented Dec 2, 2021

@WouterJD Thank you for collecting all the info in one place!

Currently, I think that we should be aware that we read data from the USB-device driver that in it's turn reads data from the USB-dongle. I would expect the USB-device driver to have it's own process, handling the USB-dongle and provides data to FortiusAnt when we do the read-command.

In line with that thought - but I do not know device driver internals, let alone I ever created such - I would not expect a separate program-thread to help in resolving the lost packet issue.

That sounds like a reasonable assumption, but unfortunately libusb is a lot more low-level than you might expect. The USB stack is not constantly receiving data from the device and buffering it for you (like a network socket). What happens when you call read is roughly this:

  • a USB request block (URB) is allocated and prepared
  • the URB is submitted to the USB stack
  • the USB host controller initiates a read transfer (requests data from the device; USB transfers are always initiated by the host)
  • if the data is received in time, it is stored into your buffer
  • if the timeout occurs first, the driver tries to cancel the USB request

Put another way, when you call read matters (so does the timeout). This is not between your program and the driver. It directly affects the device. If you don't call it often enough, the internal buffer of your device may overflow. If you cancel the transfer (i.e. a timeout occurs), there may still be data in flight. With the libusb-win32 implementation that data will simply be lost.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

@switchabl thanks for clarification. Input for further investigation.

@switchabl
Copy link
Contributor

What I have in mind is something very roughly like this:

class clsAntDongle:
   def __init__():
      ...
      self._Messages = Queue()
      self._ReadThread = threading.Thread(target=_ReadLoop)
      self._ReadThread.start()

   def Read():
      return self._Messages.get()

   def Write(messages):
      # write directly to USB as before
      # but remove any read-related code
      for msg in messages:
         self.devAntDongle.write(0x01, msg)

   def _ReadLoop():
      while True:
         data = self.devAntDongle.read(0x81, 4096, 0) # no timeout (or some large value)
         ...
         # reassemble messages from data
         ...
         for msg in messages:
            self._Messages.put(msg)

This is of course missing a lot of detail and error handling. But it is not that complicated and it would not really change interface much. I will implement something over the weekend.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

I though of something like this, open question from my side is: are you allowed to write() when the read() is still active?

Note: the thread should be initiated in getDongle() when the dongle is found, but as said thats part of the details.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

Shall I pick this up and see where I get; then you can focus on the steering.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

PS. Is multi-threading enough or is multi-processing required?

For my reference:
https://docs.python.org/3/library/threading.html
https://docs.python.org/3/library/multiprocessing.html

@switchabl
Copy link
Contributor

switchabl commented Dec 2, 2021

Threading is fine, the thread will sleep 99% of the time anyway (waiting for data). Also, multi-processing is for people who enjoy pain. -.-

If you don't mind, I was actually hoping to take a stab at this myself. This is the main issue blocking steering and I want to see if it really solves my problem (I expect it will). I also have a very good idea what to do already. I don't think it will take me long. As you can maybe guess from the discussion, I have been there before. I have spent a lot of time reading libusb and driver/kernel code and figuring out how it actually works in detail at one point. There is no reason for you to repeat that ordeal.

Edited: HRMquestion was offtopic. To keep this discussion clean.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 2, 2021

Welcome to implement; I would think you/me would subclass the clsAntDongle and make it multithreaded.

I would expect

  • that the readThread uses a reasonable timeout and then cycles through the infinite loop to the next read AND
  • that the Write() stores the messages and that the Read() sends those after that the timeout finishes

Anxiously waiting for your code😉

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 3, 2021

Hi @switchabl Good morning.

reassemble messages from data

In _ReadLoop(), I would put the raw data in the queue and interpret the contents outside the thread, to do as less as possible.
--> In function Read() the buffer can be got from the queue replacing trv = self.__ReadAndRetry(timeout)
--> _ReadLoop() could use the ReadAndRetry() function, which has a minimum of overhead in normal operation.

Also, I would reduce logging to a minimum and define a separate flag in debug.py ANTlow = 0x100 (requiring to extend All to 0xffff) for the same reason to keep overhead to a minimum.

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 3, 2021

I will do a test with a USB-hub, bringing the dongles closer to the bike

I have done some testing;

  • My laptop is quite close to the headunit (cannot position closer) at short distance from the bikes handlebar, the USB-dongles are in the laptop. ANT+ transmission has to go from chest --> laptop.
  • Using a USB-hub, the ANT-dongles are next to the headunit on the bike's handlebar, which is quite close to the chest. ANT+ transmission has to cover a shorter distance.

When walking away from the dongles, communication is lost at 1.5 - 2m (heartrate no longer updated). The distance chest - laptop (A) is definitely in the critical area.

PS 1. While doing this test, #342 was raised and resolved; will be published later.
PS 2. ExplorAnt.py is used to inspect received data from paired devices; ExplorAnt -s is used to simulate devices.

Two conclusions:

  • having the correct hardware, a USB-hub can be used to bring the dongles closer to the master device (HRM in my case)
  • using a USB-hub can reduce the ANT+ transmission distance to improve received signal

@WouterJD
Copy link
Owner Author

WouterJD commented Dec 5, 2021

Using ExplorAnt I tried to generate many messages, shortening the 250ms loop. This was not successful, so flooding buffers was not a trivial job to verify full reception. My purpose was to see that the 900byte buffer would fill up.

@switchabl
Copy link
Contributor

1.5m range seems rather short for ANT. But I guess the HRM may use a low transmit power to conserve the battery.

I am not sure you can actually exceed 900 bytes with a normal ANTUSB2/m (but easy to be safe and use 4096 bytes buffer like the Dynastream library). Overflowing the internal buffer may happen more easily though. The ANTUSB-m has a configurable "event buffer" with a maximum of 724 bytes but you have to turn that on explicitly. With the older models the assumption is always that you read continuously. I would assume that they can buffer at least 64 bytes (USB packet size) but maybe not more than that.

I do not think "overhead" in the read thread is an issue at all. Minimizing the time between transfers matters a lot if you need to maximize throughput (e.g. USB camera with high bandwidth). But with the ANT dongle, we are talking about kb/s. The extra thread is not really about performance, it is about avoid bad read patterns (with short timeouts) that cause race conditions. So my priority is clean, readable code, not on saving a ms here or there.

@WouterJD WouterJD added future development This is not on the backlog for the coming releases and removed future development This is not on the backlog for the coming releases labels Jan 4, 2022
@switchabl
Copy link
Contributor

@WouterJD Hope you are well and enjoyed the holidays. I've been rather busy before Christmas and then been away for a couple of days. I am still on it though and will finish it when I can.

@WouterJD
Copy link
Owner Author

WouterJD commented Jan 9, 2022

Hi @switchabl best wishes for you too. I know you were busy (no Christmas holidays?) so now we can move forward again.
Many people dust off their tacxes, find FortiusAnt and react on github. So all alive and kicking👍

@WouterJD WouterJD added the future development This is not on the backlog for the coming releases label Mar 23, 2022
@WouterJD
Copy link
Owner Author

Since there is no communication here, I have marked for future development.

@WouterJD WouterJD removed the future development This is not on the backlog for the coming releases label Aug 22, 2022
WouterJD added a commit that referenced this issue Aug 22, 2022
@WouterJD
Copy link
Owner Author

WouterJD commented Mar 7, 2023

ANTloop improved in current branch and working well.
Will be published soon.

@WouterJD WouterJD added implemented and removed under investigation Being studied for implementation in next version labels Mar 7, 2023
WouterJD added a commit that referenced this issue Mar 7, 2023
* steering, integrated version #1

* steering, integrated version #2 documentation

* steering, integrated version #3 bless commandline issue solved

* steering, integrated version #4 error message update

* #341 refactor ANT loop. First version

* Some small improvements (#391)

* New reading loop (in thread) ready for test

* #404 #408 and some small corrections

* Python complied into executable
@WouterJD WouterJD closed this as completed Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request implemented
Projects
None yet
Development

No branches or pull requests

2 participants