Skip to content
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

Check and Check Messages (Issue 075) #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Bilchreis
Copy link
Contributor

I have added the check and checked messages to the specification accoding to Proposal 3 layed out in Issue 075.

Changelog:

1. Added entry for check message to message table

I have added the check message in the basic messages section, where implementation is mandatory.

2. Added Check Value section

Should the Example also include a failed check? Even though Error Replys are not yet formally introduced in the spec at that point ?

3. Added NotCheckable Error Class

New error class for check messages where the optional checkable parameter property is set to false or not present.

4. Added optional checkable parameter property

Railroad Diagrams:
The railroad diagrams in the section Message handling of buildingblocks.rst need to be updated to make the changes consistent:

  • images/defined-requests.svg
  • images/defined-replies.svg
  • images/must-accept-requests.svg
  • images/must-accept-replies.svg

Is anyone still able to generate these? The source is not included in the repo.

@markus-zolliker
Copy link
Contributor

The requirement that the 'check' message has to be implement even when not used, has to be discussed. It seems that it is just needed for getting the proper error message 'NotCheckable' instead of 'ProtocolError'.

@birkenfeld
Copy link
Member

It's also not backwards compatible.

(Although AFAIR we don't specify backwards compatibility rules anywhere yet.)

@Bilchreis
Copy link
Contributor Author

I added the changes that we agreed upon in the last meeting.

@Bilchreis
Copy link
Contributor Author

I believe I have now added all changes that came up in the june 16. meeting. Regarding the broken links, I think all links are broken in the unrendered version on Github, but they should work once the page is rendered.
I still have a couple of questions:

What about argument less commands?

  • check mod1:cmd1 null should this also work?

Should a check on a command also not depend on the current Status of the module?
I can imagine a lot of commands that might depend on the status being IDLE for command execution. Resulting in the un intuitive situation where one makes a positive check on a command, and then wanting to actually execute the command, but receiving a IsBusy error.

@markus-zolliker
Copy link
Contributor

markus-zolliker commented Jul 9, 2024

I think we said that the check message may depend on the configuration of the entire SEC node, but not on the state of the SEC node. So we might always have the situation that a change message might fail after a successful check message. We can not avoid this. Concerning the argumentless command: I think there is no benefit of making an argumentless command checkable, because it has to success always, except it is configured to fail always.

@Bilchreis Bilchreis reopened this Dec 4, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo just above the current change: at the end of line 'activat connection' to replaced by 'activated connection'.

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