-
Notifications
You must be signed in to change notification settings - Fork 3
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
Control protocol transport layer. #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: heartbeat expanded. Added Coordinator coordination (initial version). Small text refactoring. Added message without a CONNECT raises ERROR.
|
…o control-transport-layer # Conflicts: # index.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
I agree with setting the heartbeats into a TBD, and get this PR through.
Apart from minor changes I have two topics to consider:
- the message types, and reduction to
CO_UPDATE
, as per this comment - Referring to the discussion in the matrix channel regarding separation of transport and message layer, it might be worthwhile to separate the abstract transport layer protocol (which does not depend on zmq, like Directories, sign-ins and Directory-updates between the Coordinators) a bit stronger from the zmq related definitions of ROUTER and DEALER sockets. However, I think we can go through with this PR, and as one of the next steps make this separation more clear in a new PR
Co-authored-by: Benjamin Klebel-Knobloch <[email protected]>
I managed to configure readthedocs builds (configured to fail on warning), and managed to activate the "list of unresolved conversations" so we have an easier time finding out how many are still open. |
There are some problems with some references/labels, I also see them when building locally.
Probably we have to check in myst-parser docs how to refer to autogenerated labels in the same file. Tomorrow.. ^.^ |
I tried to fix it and somehow, sometimes the auto-slugs to not work (despite being shown with |
turns out, for the autoslugs you always have to give the filename too, even when it's a reference to inside the same file. So, in control_protocol.md,
does not work, but
works as expected. This one: Thinking about it, both things make sense, just not yesterday night :D |
Btw, here is the link to the generated documentation (also in the first post): https://leco-laboratory-experiment-control-protocol--38.org.readthedocs.build/en/38/control_protocol.html @bilderbuchi thanks for making the rendering possible and thanks for the explanation regarding the links (I looked at myst parser and was puzzled at first, we we use I removed the Coordinator's addresses and Component connection ID from the local Directory, such that the Directory is independent of the Transport Layer's underlying protocol (zmq). That way, we push the discussion regarding Directory updates to the Message Layer (here I only request, that they notify, not whether the notification happens via a diff update or a full version). With these changes, I hope we can finish this PR. |
In pymeasure/pyleco#1 I have a Coordinator, where you may look at the Directory implementation (there are a couple of dictionaries needed, all in the init with a comment indicating what the key and what the value is). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! I agree, I think it's time to wrap this up.
LGTM, I went through my remaining conversations yesterday.
If the conversations are resolved for you, @bklebel, you may merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry for the longer delay, I agree, I am going to merge this now
First version of the control protocol transport layer.
It is a up to date state of the current discussions regarding the control protocol in different issues.
The mermaid diagrams can be seen in #39 (in the right order)
A first rendering (if it succeeds) is in https://leco-laboratory-experiment-control-protocol--38.org.readthedocs.build/en/38/control_protocol.html
I hope I made everything right with the formatting.
Things to determine regarding content:
What are your ideas/suggestions regarding formatting:
TODO