mrrwa / NmraDcc

NMRA Digital Command Control (DCC) Library
GNU Lesser General Public License v2.1
135 stars 53 forks source link

Support for advanced consisting when FLAGS_MY_ADDRESS_ONLY is set #50

Closed chrisridd closed 2 years ago

chrisridd commented 3 years ago

DCC supports a decoder having a temporary secondary "consist" address in CV 19.

Would it be reasonable for the FLAGS_MY_ADDRESS_ONLY check around NmraDcc.cpp:966 to also check against this consist address? You might need a new method similar to the existing getMyAddr() that looked at an extra cv19 field in the DccProcState.

Mr-Equinox commented 3 years ago

I was looking at how Advanced Consisting for Multifunction decoders could be implemented and this is my take:

NmraDcc.h: #define CV_MULTIFUNCTION_CONSIST_ADDRESS 19

NmraDcc.cpp:


uint16_t getMyAddr(void)
{
  if( DccProcState.myDccAddress != -1 ) // See if we can return the cached value 
    return( DccProcState.myDccAddress );

  if( DccProcState.cv29Value & CV29_ACCESSORY_DECODER )  // Accessory Decoder?
  {
    if( DccProcState.cv29Value & CV29_OUTPUT_ADDRESS_MODE ) 
      DccProcState.myDccAddress = ( readCV( CV_ACCESSORY_DECODER_ADDRESS_MSB ) << 8 ) | readCV( CV_ACCESSORY_DECODER_ADDRESS_LSB );
    else
      DccProcState.myDccAddress = ( ( readCV( CV_ACCESSORY_DECODER_ADDRESS_MSB ) & 0b00000111) << 6 ) | ( readCV( CV_ACCESSORY_DECODER_ADDRESS_LSB ) & 0b00111111) ;
  }
  else   // Multi-Function Decoder?
  {
    if( readCV(CV_MULTIFUNCTION_CONSIST_ADDRESS) & 0b01111111 )  // Consist Address?
      DccProcState.myDccAddress = readCV( CV_MULTIFUNCTION_CONSIST_ADDRESS ) ;
    else if( DccProcState.cv29Value & CV29_EXT_ADDRESSING )  // Two Byte Address?
      DccProcState.myDccAddress = ( ( readCV( CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB ) - 192 ) << 8 ) | readCV( CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB ) ;
    else
      DccProcState.myDccAddress = readCV( 1 ) ;
  }
  return DccProcState.myDccAddress ;
}
chrisridd commented 3 years ago

The github code formatting's gone a bit wild (use three backticks to surround a code block, not one), but it seems to me like once you do have a consist address that code will only respond to that address. I think it needs to be able to respond to its normal address and the consist address - how else would you dynamically remove a decoder from a consist? Well that's my logical argument; I don't know what the NMRA says.

I think having a separate getMyConsistAddr() method is needed, so the caller can do something like:

  else if( ( DccProcState.Flags & FLAGS_MY_ADDRESS_ONLY )
            && ( Addr != getMyAddr() || Addr != getMyConsistAddr() )
            && ( Addr != 0 ) )
chrisridd commented 3 years ago

Minor nit: calling readCV() looks expensive as it goes to the EEPROM, so we should try to avoid reading CV 19 twice in succession.

Mr-Equinox commented 3 years ago

Thanks, it's my first time using GitHub so unaware of the three backticks. The code block has been updated.

My understanding is that the decoder will only respond to the consist address if one is configured in CV19. To add a decoder to a consist give CV19 a value between 1 and 127. To remove a decoder from a consist set CV19 to 0. As all decoders respond to the consist address they will all be removed from the consist.

To dynamically remove a single decoder from a consist the command station would have to manage the consisting.

(I have had fellow modellers complain that their loco is unresponsive on the normal short or long address, and when looking into the CV settings have found that the loco had previously been in a consist and the consist address in CV19 had not been set back to 0. Changing CV19 to 0 restores responsiveness on the normal address.)

Regarding the two readCV() calls for the consist address: I took into consideration that the cached DCC address is most likely to be read during operation, and the two readCV() calls will only be used if the decoder is actively in a consist and CV29 or an address CV is changed (not good practice as the CV would change on all decoders in the consist) forcing a direct read of the decoder address.

Also, writeCV() will also need to be updated to reset the cached myDccAddress, to take into account CV19 changes, such as:


  {
    case CV_29_CONFIG:
      // copy addressmode Bit to Flags
      Value = Value &  ~CV29_RAILCOM_ENABLE;   // Bidi (RailCom) Bit must not be enabled, 
                                            // because you cannot build a Bidi decoder with this lib.
      DccProcState.cv29Value = Value;
      DccProcState.Flags = ( DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS);
      // no break, because myDccAdress must also be reset
    case CV_ACCESSORY_DECODER_ADDRESS_LSB:  // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS
    case CV_ACCESSORY_DECODER_ADDRESS_MSB:
    case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB:
    case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB:
    case CV_MULTIFUNCTION_CONSIST_ADDRESS:
      DccProcState.myDccAddress = -1;   // Assume any CV Write Operation might change the Address
  }
chrisridd commented 3 years ago

I went and had a look at the NMRA spec for consists, and it looks a little more complicated than I thought. See lines 160ff of https://www.nmra.org/sites/default/files/s-9.2.1_2012_07.pdf

So in consist mode, the decoder should only react to speed/direction instructions addressed to the consist.

Also in consist mode, the decoder should react to function (F1-F12, FL) settings sent to the non-consist "baseline" address, but this is controlled by a bit mask in CV 21 and CV 22.

Mr-Equinox commented 3 years ago

Hmm, yes it is more complex than simply responding to a locomotive address or a consist address. When in a consist, a decoder has to be able to respond to function commands sent to the locomotive and the consist address.

For CVs 21 and 22: https://www.nmra.org/sites/default/files/standards/sandrp/pdf/s-9.2.2_decoder_cvs_2012.07.pdf

For each Bit a value of "1" indicates that the function will respond to instructions addressed to the consist address. A value of "0" indicates that the function will only respond to instructions addressed to the locomotive address.

So, this is my understanding:

Perhaps within NmraDcc:

The decoder sketch would have to handle:

kiwi64ajs commented 3 years ago

This would be a pretty comprehensive change - feel free to submit a PR