Cleanups, code deduplication, improvements and maintenance #38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
hnez
force-pushed
the
maintenance
branch
7 times, most recently
from
8dbe76e to
e0ded51
Compare
hnez
force-pushed
the
maintenance
branch
2 times, most recently
from
b4c8cae to
257e5f5
Compare
|
I've rebased on top of recent master and tested this pull request with a couple of LXA Ethernet Muxes (because I did not test those previously). |
|
In commit "lxa_iobus: node: replace ad-hoc struct parsing with central object_directory": Sounds like there is one |
SmithChart
requested changes
Mar 20, 2024
There was a problem hiding this comment.
This is quite a large change-set. For what I can tell most looks good.
@hnez Thanks for testing. Once my two comments have been resolved we can get this merged.
Signed-off-by: Leonard Göhrs <[email protected]>
Signed-off-by: Leonard Göhrs <[email protected]>
Signed-off-by: Leonard Göhrs <[email protected]>
Signed-off-by: Leonard Göhrs <[email protected]>
The _sdo_queues dict ist only written to, but never used. Signed-off-by: Leonard Göhrs <[email protected]>
The try: except: block must have been intended to catch the case where node_id is not in self.nodes (since set_sdo_response does not do any relevant indexing), but since the access is only done after checking if the dict contains node_id this is redunant. Remove the block. Signed-off-by: Leonard Göhrs <[email protected]>
Before this any consumer of the node list had to know that node 125 is special (because it does not behave like the others and is most likely not even present). Instead make the isp node special by not adding it to the node list at all. Signed-off-by: Leonard Göhrs <[email protected]>
This improves the usecase where the Network class should be used outside of the server (e.g. in the flashing commandline utils or when using lxa_iobus as a library), because it means that e.g. await_interface_is_up() can be used before run() got around to setting the _interface_state. Signed-off-by: Leonard Göhrs <[email protected]>
This means we do not have to spawn two extra threads and can instead stay in async land. The `if True:` is only there to separate the formatting change from the code change and will be removed in a followup commit. Signed-off-by: Leonard Göhrs <[email protected]>
…mmit No actual code change, just a reduction of indentation level. Signed-off-by: Leonard Göhrs <[email protected]>
Filesystem access is generally a blocking operation and some low latency or high performance software needs to take that into account. We are neither. The single connect client can likely accept the millisecond latency spike when the lss cache file is written to disk whenever a not previously seen node is connected for the first time. The `if True:` is only there to separate the formatting change from the code change and will be removed in a followup commit. Signed-off-by: Leonard Göhrs <[email protected]>
…mmit No actual code change, just a reduction of indentation level. Signed-off-by: Leonard Göhrs <[email protected]>
…rectory Previously unpacking and packing structs to send to the node was always done ad-hoc, where it was needed. This required a deep knowledge of the different IOBus protocols whenever any of the node handling code should be changed and made it difficult to use lxa_iobus as a library outside of the server, because there was no complete documentation of IOBus CANopen endpoints. Centralize the IOBus protocol handling in a single object_directory.py file. This also changes the way IOBus protocols on nodes are enumerated. Now any node that advertises support for a specific protocol will appear in the web interface with a list of inputs/outputs/adcs it has and not just as an empty dummy node. This change also removes the notion of a "Node Driver". A node is just a collection of the IOBus protocols / CANopen endpoints it supports. The drivers only served to give the inputs and outputs and the node itself human readable and descriptive names. This is now done in a list of products in products.py. A node can however be perfectly usable without appearing in the products list, and will just use default names for all inputs and outputs. Signed-off-by: Leonard Göhrs <[email protected]>
Previously a node could be accessed via e.g. the API before it was fully set up (e.g. before the supported protocols were enumerated). This looked a bit weird in the web interface but did not do serious harm because enumartion usually happened quite fast. Completely prevent the half-initialized nodes from appearing in the web interface so that the presented state is always consistent, by only adding the node to the list once it is fully set up. Signed-off-by: Leonard Göhrs <[email protected]>
This can e.g. happen if a node was assumed to be unavailable but actually only needed a really long time to respond. In practice this should not happen, which means we want to want to be informed about it even more. Signed-off-by: Leonard Göhrs <[email protected]>
…here When specifying a struct format without a "@,=,<,>!" prefix python will default to native struct encoding with native byte order, alignment and sizes. Which is what we want if the native byte order is little endian, the alignment is "close enough" and the sizes are not crazy. This did not seam to affect anyone yet, because we do not rely on alignment to much and most systems are little endian and not crazy. But we should not rely on that and explicity state the format we want so the one person with a big endian MIPS box can use the IOBus server. Signed-off-by: Leonard Göhrs <[email protected]>
The CanIsp class has gone through a few iterations as a commandline tool using the external canopen library before it was included in the server and used the lxa_iobus intenal async canopen implementation. This is why it stayed non-async until now, making integration with the rest of the IOBus server cumbersome. Make CanIsp async-native. Signed-off-by: Leonard Göhrs <[email protected]>
…iobus We already have a perfectly working implementation of a CANopen bus and node handling, so we can de-duplicate the code we use in the commandline utils and reduce our reliance on the external canopen library. Signed-off-by: Leonard Göhrs <[email protected]>
We already have a firmware flashing implementation in the LXA IOBus server, so we do not have to duplicate it again in the commandline tool. Signed-off-by: Leonard Göhrs <[email protected]>
Good catch, should be fixed. |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just like the diffuse PR title implies this is a collection of changes that I have accumulated while working on the LXA Optick support, which itself is added in another pull request.
TODO before merging: