Discussion: refactoring/rearchitecting to separate concerns

[ianmcorvidae]

The current codebase has a lot of different things interleaved in often unpredictable ways; for example, stuff that controls how data is printed to the console being in the same basic areas of the code as sending and receiving data, and both of those together with code that controls things like managing client connections.

We should more clearly separate these things for a couple reasons:

• those using the library to write larger applications that do things very differently from a CLI shouldn't be subject to how data is printed to a console, but should be able to reuse convenience functions around things like administration, sending different sorts of data formatted correctly, etc.
• those writing separate special-purpose CLI scripts can benefit from some parts of our CLI code (for example, setting up an appropriate client from host/port/BLE options, or waiting for acknowledgements) but not need/want everything we do right now.
• it's a lot easier to maintain something where it's easier to find each bit of code!

I think this might get broken up a few different ways and I'm not sure exactly what I think should be done here, but I wanted to get the issue out here to discuss things.

Closely related to #569 since separating the concern of presentation from that of acquiring data is key to that.

Previously: #566

[holdenweb]

I would suggest we consider creating/consolidating a set of lower-level primitives from the existing wireless driver code and look at embedding this tried-and-tested functionality in a brand new framework created to a more usable command-line environment, using cleaner internal data structures and (I suspect) greatly simplifying much of the logic.

The first step will therefore be to define the radio API (my familiarity with the existing software isn't yet good enough for me to confidently say what parts currently exist) for the layers above to use. There's some milage in that discussion.

[ianmcorvidae]

So, several months later, but I think I have a bit of a clearer idea of what can happen here.

Our most core/fundamental interaction with the radio is, of course, FromRadio and ToRadio packets. MeshInterface._handleFromRadio basically is in charge of the former and is called by the various subclasses when they receive FromRadio packets, while the latter is handled by MeshInterface._sendToRadio, which calls out to _sendToRadioImpl, which is expected to be implemented by each connection type.

There's a couple user-facing APIs that are built up here, but so far only on the FromRadio side -- _handleFromRadio manages updating most of the internal data structures (primarily the nodes dict and and some things like configs and channels), and also sends a lot of the pubsub messages we send, especially within _handlePacketFromRadio which is what sends all the meshtastic.receive messages and those below that. This code is also what transforms the packets into dicts, rather than passing around protobufs. Within what's covered so far, we also have a few things that send packets out, like _startConfig, which subclasses are expected to call.

The other side of the user-facing APIs is mostly the sendData function for sending a MeshPacket to the radio, which calls to _sendPacket which calls _sendToRadio (and has some things like registering response handlers, which could possibly use a more powerful API), plus the newly-added sendHeartbeat, which sends a heartbeat message instead. Similar methods could probably sensibly be added for other things that are allowed on ToRadio -- primarily, this would probably be mqttClientProxyMessage and xmodemPacket, since want_config_id and disconnect are internal sorts of things (the latter is included within close, which can also be considered user-facing API, I suppose).

This plus some functions those things call is probably the base layer of the API, with the inclusion of the several interface subclasses. There is however a lot more in MeshInterface. The next level up, as I see it, is probably a variety of convenience functions. sendText, sendPosition, sendTraceRoute, and sendTelemetry fit here, as do the various helper functions that make it easy to pull in local information, such as getMyNodeInfo, getMyUser, getLongName, etc. These functions should all be separated out, but also made to not print out information to the console. In this same vague level of abstraction is the whole Node class, which should similarly be made to be non-printing by default. One complication here is that the Node class uses response handlers to update its own internal structures such as configurations, channels, etc. I'm of the opinion that we should probably refactor the Node to be something that can be stored in the nodes structure on the interface, and then move this handling to the low-level handling mentioned above. We should also split out things that can sensibly be done to both the local and remote nodes from those things that only make sense locally, probably by having the local node use a special subclass of Node. Relatedly, handling of acknowledgements should probably be pushed downwards into the automatic handling.

The level above this is basically the CLI itself. Most of the response handlers fall within this (except for some things that should probably be updated automatically, like acknowledgments and updates of data structures), and certainly anything that prints to the console does. I think it's acceptable to have the level below this check for things at some known names (for example, using self.onResponseTraceRoute when sending traceroutes), so long as those values are initialized to None. Then, subclasses can override these handlers for their own particular needs (such as the CLI using it for printing data out -- and perhaps a second subclass for easily parseable output). Library users should not need to directly use these implementations at all, but may use them as references for their own implementations. An equivalent sort of split should be made for nodes, as mentioned in the paragraph above.

I don't doubt that there's a few things that fall slightly outside of this structure, but I think this three-tiered model might be a good place to start as far as this kind of separation.

One issue with this is the possibility for a proliferation of subclasses, especially for the different connection types. I think we probably would not want to have CLISerialInterface, CLIBLEInterface, CLITCPInterface, FullSerialInterface, FullBLEInterface, FullTCPInterface, SimpleSerialInterface, SimpleBLEInterface, and SimpleTCPInterface (those names being examples, not proposals). It'd be a bigger change, but I think it would be positive to separate the notion of a specific kind of connection from general mesh behaviors. So rather than using subclassing, we would have some classes like SerialConnection, BLEConnection, and TCPConnection that would only expose a few things (mostly just ToRadio and FromRadio handling, plus any special initialization or finalization). Most behavior would live in the three-tiered model listed above, and those interfaces would expect a connection class to get passed in. So, rather than something like SerialInterface('/dev/ttyUSB0'), the API would be something more like CLIInterface(SerialConnection('/dev/ttyUSB0')) (or swap out for TCPConnection or BLEConnection, or swap out for FullInterface or SimpleInterface, or some combination. We could possibly, if we wanted to, retain the existing interface classes as special aliases to a particular combination of options, for backwards compatibility.

Curious if the above makes any sense and what thoughts folks have on this structure.

[broglep]

I wasn't aware of those ideas here and I ended up with something pretty similar what you are describing here (it is implemented in https://github.com/broglep/homeassistant-meshtastic/tree/dev/custom_components/meshtastic/aiomeshtastic)
I take this as a sign that should be a good structure when two individual come up independently with something similar.

I kind of see the lowest level as the transport layer, where we implement specifics for serial, bluetooth, etc.
One layer higher there is the connection layer, and there is likely only needed one implementation that makes use of the transport to provide a common interface regardless of the underlying transport.
On top of that, you have the interface that implements all the meshtastic specifc behaviour, ideally it should provide a stable interface for other consumers on top and should be independent of the actual protobuf protocol. But I found the separation between connection and interface layer quite hard to decide what functionality belongs to what, so maybe they are not necessary to separate layers. Up until connection layer the protobufs can probably serve as data objects, but I think the MeshInterface will need it its own data objects to achieve independence from connection/transport layer and offer stable interface (and hide away protobuf specifcis).

One such example I stumbled upon was the NodeInfo, that you end up having in 3 different variations (full node info as part of initial connection, partial node info send via mesh as app payload, "aggregated" node info as dict inside MeshInterface). As a consumer of MeshInteface you actually don't care about those details. Another such low level things that should not be exposed via MeshInterface are things like node id values / strings for broadcast messages that are clearly implementation details.

In my home assistant integration, I even have one layer more where meshtasic world meets the home assistant world and translation between those two worlds happen. I would see a text based cli on that same level