search

rsmp-nordic / rsmp_sxl_traffic_lights

RSMP Signal Exchange List (SXL) for Traffic Controllers
MIT License
12 stars 4 forks source link

M0020: status True/False mixed up? #191

Open wWestendorf opened 6 months ago

wWestendorf commented 6 months ago

In the current draft of SXL 1.2.1 specification, the status of M0020 is described as “False: Force output” and “True: Release output” (see table 2.85). However, there seems to be some confusion, similar to what occured with M0019. The meaning of the status in command M0019 was previously discussed [121] and updated with SXL 1.1. For consistency, the value for the status in M0020 should also be updated to “False: Release output” and “True: Force output”. What are your thoughts on this?

otterdahl commented 6 months ago

Thank you for reporting this. It indeed looks like a mistake similar to what occured with M0019. I think we should fix this in order to stay consistent.

However, I don't think this change is backwards compatible since it is likely manufacturers already implemented this. In order to stay consistent with our policy regarding semantic versioning we should wait with a fix until the 1.3 release.

@emiltin Your thoughts?

emiltin commented 6 months ago

However, I don't think this change is backwards compatible since it is likely manufacturers already implemented this. In order to stay consistent with our policy regarding semantic versioning we should wait with a fix until the 1.3 release.

Strictly speaking, a breaking changes should go in a major version:

MAJOR version when you make incompatible API changes MINOR version when you add functionality in a backward compatible manner PATCH version when you make backward compatible bug fixes

For M019 we looked at how it was actually implemented. And in fact it was implemented the natural way, not according to the spec. So the change in the spec could be seen simply as fixing a typo.

I suggest we look how suppliers have actually implemented M0020. Hopefully the situation is the same as with M0019, and we can fix this typo in 1.3.

otterdahl commented 6 months ago

Strictly speaking, a breaking changes should go in a major version:

You're right.

For M019 we looked at how it was actually implemented. And in fact it was implemented the natural way, not according to the spec. So the change in the spec could be seen simply as fixing a typo.

Yes, that's what we thought at first. However, we later learned that the exact behaviour depends on a newly added configuration option introduced in the manufacturers rsmp service in the TLC. It allows you to toggle the behaviour to either follow the earier definition of M0019 (with the mistake) or the newer definition of M0019 (fixed version).

This is not ideal of course. It introducues yet more potential error sources and it would be better to follow the SXL version numbering to define behaviour. But in general, it demonstrates how the manufacutreres may deal with a new SXL version when fix something existing in a non-backwards compatible way.

emiltin commented 6 months ago

Yes that's adding yet another complication. I hope we can fix this in 1.3, but it might be prudent to check whether it would cause issues with current implementations.

emiltin commented 2 months ago

Arguable, a bug fix requires just a new patch version, not a major version.

emiltin commented 3 days ago

We have a test for M0020 in https://github.com/rsmp-nordic/rsmp_validator/blob/59b52e3fd7a541a70ac73a6529c5cf1616aeeea6/spec/site/tlc/io_spec.rb#L146, but this test simply checks that you can force/unforce, but does not yet check behaviour.

emiltin commented 3 days ago

PR in https://github.com/rsmp-nordic/rsmp_schema/pull/117. If we agree merge this, the change then needs to be pulled into the TLC SXL repo.