Skip to content

Conversation

geeksville
Copy link
Member

@geeksville geeksville commented Apr 27, 2024

Some time ago I updated the protocol so that after sending NODEINFOs to client devices (via fromRadio protobufs), it would proactively send the channel definitions. Prior to this the client had to ask for each channel by sending an admin request. This handshaking was really slow for mobile/ bluetooth clients. So in the current device code (last couple of years) the device automatically sends this info to the client during initial config download.

Unfortunately I never updated the python client to expect this. I assumed (incorrectly) that it would just cope and keep sending the channel requests the old way it always had (which is still supported by devices - and required for remote administration of nodes).

This change removes sending channel requests (for the local node only) from the python startup code. It also now understands (no longer ignoring) the channels which were sent proactively by the local device.

This makes connection/config/api operations from the python client 100% reliable again. Previously it would only work sometimes depending on how quickly it was able to get the local node db downloaded. This was especially obvious if using the wifi interface to talk to the meshtastic node.

Also this flow is much faster - which should help all users of the python api (even when connecting locally via serial/USB).

Old behavior

about 50% of connection attempts would fail.

 meshtastic --host 192.168.86.169
 ... Error connecting to 192.168.86.169:Timed out waiting for connection completion

New behavior

100% of attempts succeed

Some time ago I updated the protocol so that after sending NODEINFOs to
client devices (via fromRadio protobufs), it would proactively send the
channel definitions.  Prior to this the client had to ask for each channel
by sending an admin request.  This handshaking was really slow for mobile/
bluetooth clients.  So in the current device code (last couple of years)
the device automatically sends this info to the client during initial config
download.

Unfortunately I never updated the python client to expect this.  I assumed
(incorrectly) that it would just cope and keep sending the channel requests
the old way it always had (which is still supported by devices - and
required for remote administration of nodes).

This change removes sending channel requests (for the local node only)
from the python startup code.  It also now understands (no longer ignoring)
the channels which were sent proactively by the local device.

This makes connection/config/api operations from the python client 100%
reliable again.  Previously it would only work sometimes depending on how
quickly it was able to get the local node db downloaded.

Also this flow is much faster - which should help all users of the python
api.
Copy link

codecov bot commented Apr 27, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 61.89%. Comparing base (3d6fa36) to head (3886bc1).
Report is 1 commits behind head on master.

Files Patch % Lines
meshtastic/mesh_interface.py 37.50% 5 Missing ⚠️
meshtastic/node.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
- Coverage   61.94%   61.89%   -0.06%     
==========================================
  Files          14       14              
  Lines        2917     2926       +9     
==========================================
+ Hits         1807     1811       +4     
- Misses       1110     1115       +5     
Flag Coverage Δ
unittests 61.89% <36.36%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@geeksville geeksville changed the title Update python client to use the 'modern' meshtastic protocol init flow (high-pri fix) Update python client to use the 'modern' meshtastic protocol init flow Apr 27, 2024
@ianmcorvidae
Copy link
Contributor

This is excellent, something I was theoreticallly in the middle of working on when you came by, so very happy to see it done and not waiting on me to get it done! I'll merge now since as you point out it's pretty bad that we've been getting all the channel info twice every run 😖

@daytonturner
Copy link

I apologize for jumping aboard this PR thread to figure this out, but I've been stumped on it for 3 days now and I would love to understand what I'm missing.

I've been chasing down what seemed to be timeouts while interacting with the TCP API (meshtastic/firmware#3618) - initially thought to be specific to the RAK 13800 Ethernet, but later suspected to be any TCP API client over any medium. This update seems to have eliminated the timeouts we were experiencing, but after trying to understand why the failure was occurring in the first place, I'm still confused by the ServerAPI code:

https://github.com/meshtastic/firmware/blob/master/src/mesh/api/ServerAPI.cpp#L48-L59

Simple enough at first glance, if there's already an existing TCP API connection and a new one comes in, close the old one and connect the new one.

The confusing part was chasing down how line 48 calling U::available() works - it seems to return the socket index for any established connection with contents in the read buffer. However, this loop runs every 100ms, and seemed to be responsible for closing an existing connection when it took too long. The CLI requesting --info or --nodes would take around 3 seconds, and still does. But, now it doesnt early-terminate the socket.

What about ::available(), the 100ms loop, and the force-closing am I not understanding?

(Thank you for the fix, by the way - great PR!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants