adding antplus, first working version #53
Conversation
There was a problem hiding this comment.
Hi! I'm not actively using ant-fs but I'll try to dig out my old watch and see if this works. I've mostly tested with one device so I can understand that things don't work as well with multiple devices. The core isn't as solid as I would have wanted it to be either, or how I would write it today - there are some plans for rewriting it of course but since I don't actively use it I haven't gotten around to it.
Python 2.7, 3.4 and 3.5 can be removed since they have reached end of life. I fully support that. Also nice if you can start using a few newer things with that removal.
I haven't had time to look through everything yet, so I'll get back to you, next week - hopefully.
.travis.yml
Outdated
| # older versions of python don't support f-strings | ||
| # - 2.7 | ||
| # - 3.4 | ||
| # - 3.5 |
There was a problem hiding this comment.
These versions are not supported anymore so they can be removed.
There was a problem hiding this comment.
I've removed these version now, as well as adding the black formatter (sorry) if you want to rebase.
There was a problem hiding this comment.
Btw, let me know if you need assistance in rebasing.
There was a problem hiding this comment.
a bit of a pain as you can see below :/
last build was ok after installing black formatter
there is an issue in your ant.py line 180 & 186, please check.
stefaandesmet2003
force-pushed
the
master
branch
from
8402f74
to
960c246
Compare
stefaandesmet2003
force-pushed
the
master
branch
2 times, most recently
from
37b5f0f
to
1997f98
Compare
stefaandesmet2003
force-pushed
the
master
branch
from
1997f98
to
6dfa2ae
Compare
| self._running = False | ||
| self._worker_thread.join() | ||
|
|
||
| # SDS - releasing resource (serial/usb) |
| ): | ||
| _logger.debug("Got channel event, %r", message) | ||
| self._events.put( | ||
| ( | ||
| "event", | ||
| (message._data[0], message._data[2], message._data[3:]), | ||
| (message._data[0], message._data[1], message._data[2:]), |
There was a problem hiding this comment.
Shouldn't this be (message._data[0], message._data[2], message._data[3:]), as it where? aka channel (0), event (2) and rest data?
There was a problem hiding this comment.
This line changed in your last commit, and it raised an exception on receiving a channel event, because message._data array had only 3 bytes : (channel, 1,event_code) (ant protocol §9.5.6.1). And the rest of your code, especially wait_for_event in filter.py, expects the event tuple to be exactly this. The filter.py code is a bit hard to read, but this tuple is translated to (channel, event,data), with event always = 1 (channel event, not response), and data[0] corresponding to event_code. The code works even if event parameters are sent, which my device doesn't do.
| # SDS WORKAROUND : eerst kijken of nog uitstaande TX_COMPLETED events in de node._events zitten voor dit channel | ||
| # voor het geval er nog een is aangekomen buiten timeout van een vorige send_acknowledged_data | ||
| # momenteel verhindert niets om meerdere send_acknowledged_data op hetzelfde channel terzelfdertijd te starten vanuit <> threads | ||
| # dus deze workaround kan deze events wel onterecht wissen! | ||
| self._node._event_cond.acquire() | ||
| msgs_to_delete = [] | ||
| for message in self._node._events: | ||
| if (message[0], message[1], message[2][0]) == ( | ||
| self.id, | ||
| 1, | ||
| Message.Code.EVENT_TRANSFER_TX_COMPLETED, | ||
| ): | ||
| msgs_to_delete.append(message) | ||
|
|
||
| for message in msgs_to_delete: | ||
| self._node._events.remove(message) | ||
|
|
||
| self._node._event_cond.release() |
There was a problem hiding this comment.
Could you explain this bit? My Dutch is a bit rusty :).
There was a problem hiding this comment.
Haha! First of all, I apologize for the dutch, should have cleaned that. If a TX_COMPLETED event arrives after the timeout, the event will end up in the node._events queue, and a subsequent send_acknowledged_data will look in the queue, find a TX_COMPLETED event, but not the right one, and unblock prematurely, even before anything is sent on RF, let alone an ACK received. The cause is the wait_for_message function, that doesn't match events with the channel; and doesn' guarantee a minimum timeout : for _ in range(10): when a burst of 10 events arrives, even on a different channel, the timeout is quasi nihil. In my case the events were especially QUE_OVERFLOW and EVENT_QUE_OVERFLOW that flooded in, until I removed the sleep(0.1) line in ant.py and the usb-read thread could handle all the incoming data.
The solution needs rework on different files, and haven't gotten there yet. The workaround simply checks if there are trailing TRANSFER_TX_COMPLETED events in the queue, and deletes them.
| # SDS BUG : need to check for actual channel number, but it's not passed in | ||
| # SDS : original code temp disabled | ||
| """ | ||
| elif message[1] == 1 and message[2][0] in [Message.Code.EVENT_TRANSFER_TX_FAILED, | ||
| Message.Code.EVENT_RX_FAIL_GO_TO_SEARCH]: | ||
| _logger.warning("Transfer send failed:") | ||
| _logger.warning(message) | ||
| queue.remove(message) | ||
| condition.release() | ||
| raise TransferFailedException() | ||
| """ |
There was a problem hiding this comment.
not in my latest commit, I'm afraid. I've only highlighted that there is an issue with the filter.py code, because it should also match the event with the right channel. For instance RX_FAIL_GO_TO_SEARCH could be a misconfigured channel from another device.
| # SDS TODO : hier de self._events opkuisen/afhandelen | ||
| # dan heeft die timeout op datas.get() nog zin | ||
| # beter om dit per channel te doen ipv de volledige node? | ||
| # SDS : voorlopige workaround | ||
| # remove RX_FAILED, COLLISION events that keep piling up | ||
| # TODO : nog niet ok, want als het [0] niet wordt gecleaned of gebruikt, stapelen de vuile events zich erachter op | ||
| # hier moet een algo komen die systematisch alle events afhandelt! | ||
| _logger.debug("SDS - cleaning node._events queue %r", self._events) | ||
|
|
||
| # sds de .acquire() is niet echt nodig als je events.remove(message) gebruikt ipv popleft | ||
| # zonder de .acquire() zou de node._events queue kunnen wijzigen tussen message=... en node._events.popleft() | ||
| # om dezelfde reden kan je ook geen for message in node._events loop doen zonder lock acquire | ||
| # deze cleaning vermijdt nog niet dat wait_for_event wordt genotified bij elke inkomend event (ook die van <> channel) | ||
| # want deze cleaning routine krijgt niet noodzakelijk eerst toegang tot de gewijzigde node._events | ||
| # dit is sowieso tijdelijk, maar eigenlijk kan je ook beter een lijst hebben van events die moeten blijven ipv op te sommen wat weg moet | ||
| # ofwel moeten we hier die error events callbacken naar de applicatie |
There was a problem hiding this comment.
I noticed that the node._events queue grows continually during execution, because the events are never cleaned. I'm not an expert in ANT, but I would expect that RX_FAILED and COLLISION events are totally normal when multiple channels are open. Every channel has a fixed channel_period; even if they're slightly different, at some point in time they will overlap and raise a COLLISION or RX_FAILED event. Same with QUE_OVERFLOW and other error events, what do you want to do with these?
And then the TODO says that the current quick hack algorithm is flawed, because if the [0] event cannot be handled, the unhandled events will pile up after it, and the cleaning doesn't work. Like I warned you, it's work in progress ... My first goal was to keep the code running. It looks ugly, I admit
your review is much appreciated! |
|
Is this active ? Any need for some help ? |
|
Thanks for the work on this but closing as ANT+ device support is added with #76 and this is now out of date. |
|
Sorry, but I think @stefaandesmet2003 version seems much more cohesive, complete and scalable, imo. |
|
Fair enough and no need to be sorry, I don't take it personally! Pity you didn't see my PR to comment then, but open to PRs to improve on what I've done. |
Hi,
offering here a look at my work in progress on adding support for ant+ in your repo.
I added details in the readme.md about the changes I made and why. main reason for the changes is support for multiple open channels (for instance FE-C + cadence sensor, or HRM), which didn't work well with your code base.
Unfortunately I cannot test if my changes break functionality on ant-fs, I don't have a suitable device.
I don't expect a break, only doubt is the time.sleep(0.1) you had after every received broadcast, and which solved a lot of my problems after I removed this line.
forget about the changes in travis.yaml, I removed python<3.6, because I had f-strings in the logging code, and only realized the consequences after setting up travis & coveralls this morning.
cheers,
Stefaan