Closed BenediktBurger closed 1 year ago
Very nice work! By and large I agree with the meaning I understood, I tried to only comment on high-level/general and/or some clarity issues. Good of you to call out what you want to focus on! When this has settled I can do some editing passes..
Thanks for your review.
Sorry for the misunderstanding in that one graph.
I reworked the diagrams, especially the "message flow diagram". I hope, it is now clearer (together with the legend written above it).
I also added the SIGN IN to another Coordinator.
Except for above mentioned (small) questions, the Transport Layer is done.
So, we can improve language now as well.
I updated the Coordinator-sign-in procedure according to #44 (with a few improvements). An additional, flow chart of decisions is in #39 .
I suggest, we leave heartbeats open (a TBD not is there) and finish this PR.
The heartbeat can be added in its own PR.
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.
/home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:17: WARNING: undefined label: 'appendix.md#router-sockets' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:29: WARNING: undefined label: 'coordinator-sign-in' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:91: WARNING: undefined label: 'directory' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:91: WARNING: undefined label: 'coordinator-coordination' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:125: WARNING: undefined label: 'signing-out' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:135: WARNING: undefined label: 'directory' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:135: WARNING: undefined label: 'coordinator-coordination' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/control_protocol.md:241: WARNING: undefined label: 'directory' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/glossary.md:32: WARNING: undefined label: 'namespace' /home/docs/checkouts/readthedocs.org/user_builds/leco-laboratory-experiment-control-protocol/checkouts/38/glossary.md:32: WARNING: undefined label: 'component-name'
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 myst-anchors -l 4 control_protocol.md
), sometimes the manual ones do not work (to the router socket in the appendix)...
sometimes the auto-slugs to not work (despite being shown with myst-anchors -l 4 control_protocol.md),
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,
{ref}`message-layer`
does not work, but
{ref}`control_protocol.md#message-layer`
works as expected.
This one: <appendix.md#router-sockets>
was mixing autogenerated-slug-style (filename#anchor
) and native ReST/Sphinx (directly refer the label, which has to be unique throughout the whole document), which did not work.
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 {ref}...
, while the myst documentation gives the normal markdown syntax for links, until I understood it).
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 https://github.com/pymeasure/pyleco/pull/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).
If the conversations are resolved for you, @bklebel, you may merge.
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