search

openlcb / documents

The OpenLCB specification: standards, recommended practices and other documentation.
3 stars 7 forks source link

Train Search S & TN Review - 28 June 2024 #114

Closed bakerstu closed 2 weeks ago

bakerstu commented 2 weeks ago

Here are my review comments for draft version 24 June 2024 (reviewed on 28 June 2024). Overall, very good work.

Standard

TN

bobjacobsen commented 2 weeks ago

This is really very good. Comments on the two documents follow, mostly minor and optional:

Standard:

Re "DCC - Default Address space" - Line 76 says "The Address for the DCC Protocol also carries a disambiguator its's a short or long DCC address". But it's not clear (at least to me) that "Default Address space" means "short if under 128, long otherwise". it would be good do document that, perhaps at line 76.

Related to that, some systems draw the line between short and long at e.g, 100 instead of 128. Perhaps what "Default" means is "what the underlying DCC control system uses for separation". If so, it would be good to say that.

Technical Note:

balazsracz commented 2 weeks ago

Thank you for the thorough review!

https://github.com/openlcb/documents/pull/119

Here are my review comments for draft version 24 June 2024 (reviewed on 28 June 2024). Overall, very good work.

Standard

  • Line 35 - This block of Event Identifiers is not defined in the Event Identifiers Standard. I believe it should be, under a new section called "Well-Known Other". The TN should further explain the history around these being donated out of the TCS manufacturer owned space.

Done in #115

  • Line 38 - The document that allocates this range to TCS is Unique Identifiers Standard, not Event Identifiers Standard.

Fixed.

  • Lines 38-41 - I think we should strike "Train Control Systems, Inc., or". IMHO, this should be worded in such a way that the future management of this range is designated to the OpenLCB group by TCS in perpetuity. I don't know if we need to write a contract to such effect, but we can if necessary. Maybe a memorandum of understanding is good enough.

That does not work very well from the succession planning perspective. If the OpenLCB group disappears or becomes inoperational, now TCS is by those letters unable to innovate in a protocol they designed for their devices.

  • Line 50 - I think in general, this table needs quite a bit more information in the entry descriptions.

All of the semantic information is in the Interactions section.

  • I prefer to either repeat the "value" rather than a double quote or merge the cells, as we have done in the Event Identifiers standard.
  • It is unclear what is supposed to happen with the '*' fields. Can one put any value in this field because it is ignored? How is it actually used?

I do want to keep the ability to oversee the contents of this table, therefore I am not on board with repeating every value in every row. This is also not possible in places where independent fields are described.

I removed the ", because that is not a common representation we use in our standards. I used the most common representation which is merged cells.

I removed some 's. Now there are s only in rows that act as a header for rows that are below them. This is also something we commonly do in other standards.

  • What does "DCC - Default address space" mean? Is this the same as a 7-bit address?

This is defined in the interactions section (section 6.3) ; specifically in a pretty complex set of expressions around line 140. The default address space is 7-bit for values < 128 and 14-bit for values >= 128.

  • Line 73 - The reference to "Track Protocol table" is not completely clear. Might consider adding a section reference, and possibly even making sub-sections for the different tables in section 5.2.

I added table numbers.

  • Line 104 - Should the word "setting" be removed from this sentence?

rephrased.

  • Lines 125-160 - This is really hard to read through. Might these logical expressions be easier to read in a pseudo-code notation, such as:

    • ((protocol is DCC [OR] value is >= 128 decimal [OR] ... ) [AND] (...))

I had parentheses in a previous version. I found that the nesting of multiple parentheses is way more difficult to read for a person than the current form, where indentation represents the grouping of conditions. I also found that unlike in code, parentheses read very poorly within English text, because they convey the message that the contents of the parentheses is somehow not important, whereas in reality it's exactly the opposite.

Ultimately this is hard to read because it is a complex condition. It will be equally complex in another form. The question is whether there is any ambiguity of the interpretation.

  • Line 132 - Consider not using the first person "we" pronoun.

done

balazsracz commented 2 weeks ago

Here are my review comments for draft version 24 June 2024 (reviewed on 28 June 2024). Overall, very good work.

TN

  • Very good use of sequence charts.

thanks!

  • There is a lot of first person "we" dialog. Consider modifying the text to not use first person pronouns.

I did this, but it went at the expense of readability of the text.

  • Line 182 - How is value 255 used? Does it need to be specified in the Unique Identifiers Standard?

There is an extended addressing method available for Marklin-Motorola that can represent 256 address values. Newer decoders (e.g. ESU LokPilot 3 and later) can get MM addresses above 80. I know how to generate the extra waveforms on the track, but never found documentation about which waveform is what address value.

I updated the language to 1 to 80 (or 1 to 255) for Märklin-Motorola

  • Line 404 - Remove repeat of the word "the".

Fixed

balazsracz commented 2 weeks ago

This is really very good. Comments on the two documents follow, mostly minor and optional:

Standard:

  • This doesn't define "Virtual Node". Does it need to be? It's in the common Glossary TN

I added a paragraph into the standard, at the beginning (in the informative section).

  • In Search Query nibble 0xF description: "an" in "evaluated with an AND relation"

fixed

  • In Train Protocol values table, "DCC - Default address space": Not sure if there's enough in the Standard doc. See discussion below

the normative definition is in the interactions section. I added a note to this effect. (also see replies below)

  • Line 129 "rr matches ... (iff) ... rr is set to "Default/Any or Protocol matches the value of the Track Protocol field". That seems a bit ambiguous around "MM - Any / Default version" and "DCC - Default address space". I recommend adding ", including defaults within spacific protocols" or similar.

The next line says explicitly that the protocol version is not considered during the match algorithm. The address space is considered below.

  • Line 140: Just curiousity, but why is the ">= 128" condition in there? It seems redundant, so long as the short-long divider is at 128, and incorrect when it's at 100.

If I remove that line, then the match algorithm will not match a locomotive with address 129 when the default address space is selected. So specifically the query FFF12900 will not match anything ever on DCC. (Walk through the conditions by hand and you'll see.) In other words, this line is the normative definition of what "default address space" means.

Re "DCC - Default Address space" - Line 76 says "The Address for the DCC Protocol also carries a disambiguator its's a short or long DCC address". But it's not clear (at least to me) that "Default Address space" means "short if under 128, long otherwise". it would be good do document that, perhaps at line 76.

As said above, this is defined in line 140.

Related to that, some systems draw the line between short and long at e.g, 100 instead of 128. Perhaps what "Default" means is "what the underlying DCC control system uses for separation". If so, it would be good to say that.

I don't want to leave the interpretation of long or short addresses to some implementation-defined behavior, because this would create an interoperability issue between throttles and command stations of different manufacturers. This means that it would be possible that you go with your openlcb throttle and your dcc locomotive to a different layout, put the locomotive on the track, and then you can not select it on your throttle, because that command station made a different choice than yours. This does happen on DCC and I don't want to make the same mistake.

If someone is making a lenz throttle to openlcb gateway, and want it to run according to the Lenz behavior, then they have to set the "force long address" bit when the user enters 100. This will prevent that gateway from ever addressing short address 100, but it will preserve Lenz's behavior. This will work on any OpenLCB command station.

Since this use-case is supported by the protocol, I don't need to leave the default choice as an open question.

Technical Note:

  • A minor thing, but on line 5 you reference the definitions of "Train, Throttle and Command Station" in the Standard. The Standard defines Train Node and Throttle Node.

fixed

  • Line 27 This appears to be the only occurence of "Legacy" Perhaps change that?

removed

  • Not sure where this goes, maybe under "Unserved Use cases" - This protocol does not support having multiple command stations that serve the same Protocol trains. For example, you can't have two DCC command stations attached, as both will try to create a virtual node when one of a particular DCC address is specified. (And the 200 msec wait doesn't disambiguate that completely) It would be good to mention this somewhere.

thanks, added

  • Line 33: A centralized component called a Command Station

done

  • Line 50 has a TODO to replace with a reference to Section 3

done

  • Line 83 - why is this 4 digits, not 6?

good point, fixed.

  • Line 103 and 105: Can we use a different word besides "signal"? "instruct", "command", "tell"?

done

  • Line 115, footnote 4: Add to the footnote "Note that this is an exception to the rule that Producer Range Identified should only be used to identify a range that is more than 50% used".

done

  • Line 170-173: Command station is used two ways in this paragraph. I recommend putting "proprietary" in front of the second to read "remote controlling a proprietary Command Station connected to that proprietary bus."

done

  • Bullet at line 179: This sentence no verb. Perhaps add "Protocol can be ... Space is reserved for others in the future"

done

  • Bullet point lines 182 to 192: This is really dense. I recommend putting paragraph breaks after "for Markin-Motorola" and after "those addresses by the number." That'll break it in to three semi-independent sections.

done, thanks for being specific.

  • Section 5 has several "we" pronouns that could be removed

Fixed

bobjacobsen commented 2 weeks ago

Thank you for the explanations.

I don't want to leave the interpretation of long or short addresses to some implementation-defined behavior, because this would create an interoperability issue between throttles and command stations of different manufacturers. This means that it would be possible that you go with your openlcb throttle and your dcc locomotive to a different layout, put the locomotive on the track, and then you can not select it on your throttle, because that command station made a different choice than yours. This does happen on DCC and I don't want to make the same mistake.

If someone is making a lenz throttle to openlcb gateway, and want it to run according to the Lenz behavior, then they have to set the "force long address" bit when the user enters 100. This will prevent that gateway from ever addressing short address 100, but it will preserve Lenz's behavior. This will work on any OpenLCB command station.

Since this use-case is supported by the protocol, I don't need to leave the default choice as an open question.

I see your points above.

There's one more case: I take my OpenLCB throttle to a layout operated by a DCC command station that only allows 1-100 as short addresses. How does that throttle know that I can't select 0123 and operated a short-123 address locomotive that's sitting on the track? The throttle makes the request with the long-address bit reset and the allocate bit set. The matching algorithm matches and the Command Station makes a search reply. To the user, it looks like the throttle has allocated properly (because the search found something, etc), but the locomotive won't move.

The solution is allowing the Command Station to not reply if it can't support a particular address. I think that's included in the language on line 109 "validate that it has the an ability to create a Train Node matching". If it can't handle short-123, it doesn't have to reply. But it took me a while and some close reading to figure that out.

Perhaps add to the end of TN section 2.6.2 (somewhere near line 218 in the older version): "Note that in some cases a command station is unable to control certain addresses. For example, some DCC command stations require address 123 to be a long address, and cannot control short address 123. The command station should not reply to the search query if it cannot control the requested address."

balazsracz commented 2 weeks ago

I see your points above.

There's one more case: I take my OpenLCB throttle to a layout operated by a DCC command station that only allows 1-100 as short addresses. How does that throttle know that I can't select 0123 and operated a short-123 address locomotive that's sitting on the track? The throttle makes the request with the long-address bit reset and the allocate bit set. The matching algorithm matches and the Command Station makes a search reply. To the user, it looks like the throttle has allocated properly (because the search found something, etc), but the locomotive won't move.

The solution is allowing the Command Station to not reply if it can't support a particular address. I think that's included in the language on line 109 "validate that it has the an ability to create a Train Node matching". If it can't handle short-123, it doesn't have to reply. But it took me a while and some close reading to figure that out.

Perhaps add to the end of TN section 2.6.2 (somewhere near line 218 in the older version): "Note that in some cases a command station is unable to control certain addresses. For example, some DCC command stations require address 123 to be a long address, and cannot control short address 123. The command station should not reply to the search query if it cannot control the requested address."

I hadn't thought of this opposite problem, but address out of range is definitely a case that I was worried about when I wrote the standards text (that line 109 you mentioned). This is also present in the code I wrote.

I added the commentary to the TN to this effect. There are other address out of range cases, so I added more examples.

Re: short address 123.

I presume this would happen when someone is using an OpenLCB throttle on a Lenz command station, via some form of gateway. Technically when you bring a short address 123 locomotive to a Lenz command station, you won't be able to make your locomotive move - either with your own throttle or with a Lenz throttle. Presumably also not via a WiThrottle Protocol client through JMRI, as JMRI won't be able to create the throttle object from the lenz throttle manager.

For this locomotive you are accustomed to punching in 123 on the throttle, not 0123, as 0123 would drive long address 123 even on your home layout. So you punch in 123, and you get no response, and the throttle displays an error. This is fine, it is the expected behavior. There should be an error, as you will not be able to drive the locomotive on this layout.

The big difference coded in this behavior is that if there is a local locomotive on the layout with cab number 123, on the local lenz throttle, you have to type in 123, and on your visiting openlcb throttle you have to type in 0123. This is not super awesome, but on par with what would happen if you were to take that locomotive to an NCE layout and used a hammerhead to drive it. You would have to call it up using 0123.