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

M0004, restart controller - deprecate #180

Open
sveitech opened this issue Mar 23, 2023 · 12 comments
Open

M0004, restart controller - deprecate #180

sveitech opened this issue Mar 23, 2023 · 12 comments
Milestone

Comments

@sveitech
Copy link

Since the first implementation of Swarco RSMP, the command object M0004 has only cleared ITC errors, and not restarted the device.

We know the SXL says "restart controller", but this is how the swarco implementation has been working for almost 10 years now. This particular implementation has been tested and approved by 3rd parties.

We cannot readily change this implementation as it would cause a drastic change in expected behavior.

@emiltin
Copy link
Contributor

emiltin commented Mar 30, 2023

Thank you for the input, we will discuss it.
Generally, we're moving towards stricter adherence to the rsmp spec, and flagging any behaviour that's non-compliant, whatever the historic reason.

@emiltin
Copy link
Contributor

emiltin commented Jun 1, 2023

@otterdahl input? We need to change the spec and/or the implementation, to fix this issue. I see a few options:

  • change all version of the sxl to require restart (NOT just clearing of errors)
  • change all version of the sxl to require restart OR clearing of errors.
  • change all version of the sxl to require clearing of errors (NOT restart)
  • change older version of the sxl to require restart or clearing of errors. for newer versions, require a restart (perhaps with a flag to clear errors instead of restarting).

@sveitech
Copy link
Author

sveitech commented Jun 1, 2023

For the ITC-3 controller, this object will always ONLY reset errors. We will never comply with a specification that requires the controller to restart, due to a remote command (due to safety and liability issues), and I guess other manufacturers will have the same issue.

Perhaps some commands should be optional, or new commands should be vetted by manufacturers first, before they are entered into the spec.

For the ITC-3 controller, this command has only cleared errors since it was implemented (2012) and has been approved by clients for all those years.

@otterdahl
Copy link
Contributor

In Sweden we've historically accepted that this command doesn't actually restart the controller. I thought the ITC-2 did restart in case there was serious error active in controller. But perhaps I'm mistaken.

... I guess other manufacturers will have the same issue.

I know at least one manufacturer that did implement this command and it restarted the controller immediately - without switching to all red first. Not great from a safety perspective. I don't think we've ever used this command in practice.

* change all version of the sxl to require restart OR clearing of errors.
* change all version of the sxl to require clearing of errors (NOT restart)

I think clearing/resetting errors is a different type of command and we shouldn't change the meaning of an existing command. In that case, it's better to deprecate this command in favor for a new one.

I'm in favor of consider this command as optional and perhaps add a separate new command for resetting errors.

@emiltin
Copy link
Contributor

emiltin commented Jun 6, 2023

I'm in favor of consider this command as optional and perhaps add a separate new command for resetting errors.

Then we should specify how the site should respond if a restart is not supported, eg.

  • return a NotAck with rea set to "Restart not supported"
  • should it clear errors, or not?

@emiltin
Copy link
Contributor

emiltin commented Jul 14, 2023

We can depreciate this command, and add a new command for clearing errors? It sounds like restarting is not safe and not well supported.
Even if we depreciate, it might be good to ammend previous versions of the spec to clarify behaviour in case restart is not supported (as mentioned in the previous comment).

Or maybe it's easier to just redefine the mesage to mean Clear Errors, or maybe Reset. Then existing implementations that clears errors will not have to be change.

@Henr1O
Copy link

Henr1O commented Oct 12, 2023

In La Semaforica Cartesio, this command does exactly what it should, it will restart the controller. Like Emil wrote, there should be a new command, which will only clear errors (reset alarms/errors is the same thing =)).

@emiltin
Copy link
Contributor

emiltin commented Oct 13, 2023

Thank you for the input @Henr1O, very interesting to learn about Cartesio.

If we have a separate command to rest the controller, is the restart command needed? Some venders, like Swarco, do not want to support a restart command. So we can either say that they are not compliant, or we can make the restart command optional. Or depreciate it if it's not really needed.

@Henr1O
Copy link

Henr1O commented Oct 13, 2023

For controller reboot, I don't see so much use cases. Clearing alarms (or resetting errors) command is much more useful. I agree with David to make totally new command, and leave the M0004 for optional use.

In Sweden we've historically accepted that this command doesn't actually restart the controller. I thought the ITC-2 did restart in case there was serious error active in controller. But perhaps I'm mistaken.

... I guess other manufacturers will have the same issue.

I know at least one manufacturer that did implement this command and it restarted the controller immediately - without switching to all red first. Not great from a safety perspective. I don't think we've ever used this command in practice.

* change all version of the sxl to require restart OR clearing of errors.
* change all version of the sxl to require clearing of errors (NOT restart)

I think clearing/resetting errors is a different type of command and we shouldn't change the meaning of an existing command. In that case, it's better to deprecate this command in favor for a new one.

I'm in favor of consider this command as optional and perhaps add a separate new command for resetting errors.

@emiltin emiltin transferred this issue from rsmp-nordic/rsmp_validator Nov 29, 2023
@emiltin emiltin added the extend label Nov 29, 2023
@emiltin
Copy link
Contributor

emiltin commented Nov 29, 2023

moved this issue from the validator repo to the tlc sxl repo

otterdahl added a commit to rsmp-nordic/rsmp_schema that referenced this issue Mar 19, 2024
@otterdahl
Copy link
Contributor

otterdahl commented Mar 19, 2024

@emiltin emiltin changed the title M0004, restart controller M0004, restart controller - deprecate Mar 22, 2024
@emiltin
Copy link
Contributor

emiltin commented Mar 22, 2024

milestone set to the next major, since this is a breaking change

@emiltin emiltin added this to the 2.0 milestone Mar 22, 2024
@emiltin emiltin added deprecate and removed extend labels Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants