Problems using Accessory Addresses over 252

[DonWillsmer]

I have experienced problems trying to use an accessory address over 252. I do not get the same problem using a different command station. I do not have access to another CANCMD so cannot check whether the issue affects other CANCMD or if the issue is with my machine.
This is low priority but if anyone one else is having this issue it would be helpful to know,

Don Willsmer

[MonitorComputing]

I don't have the Microchip C compiler nor a spare CANCMD to run real world test on but I have bench tested the C source for the event number to accessory address, encoded into the two byte packet format, as from the function "dccAccessoryWrite" in "packet_gen.c" on branch "V4BETA3", and can see two errors,

  ...
  s_ptr->d[0] = 0x80 + ((((acc_address.addr_lo) >> 2) + 1) & 0x3F) + ((acc_address.addr_hi.byte << 5) & 0x20);
  ...

"((acc_address.addr_lo) >> 2) + 1) & 0x3F" is getting bits 2 to 7 of the address, adding 4 (>> 2 makes 4 become 1) because the DCC addresses start at 4 rather than 0, and putting that into the first byte of the DCC packet. The problem is CBUS event 252 becomes DCC address 256 which requires a carry from into bit 8, which this code doesn't do. The addition of 4 should happen on the 16 bit DCC address before encoding to allow carries into bits 8 to 10.

" + ((acc_address.addr_hi.byte << 5) & 0x20)" is taking bit 8 of the DCC address, i.e. bit 0 of the "high" byte, and putting it into bit 6 of the first encoded packet byte. I think here the original author got themselves confused over bit numbering because bit 6 of the first encoded packet byte has already been loaded with bit 7 of the DCC address, from its "low" byte. I'm not sure exactly what problems, if any, this would cause because I'm testing on a 64 bit architecture using the Gnu compiler rather than a PIC using the Microchip compiler and had to make some tweaks to get the various unions and structures to behave, more about that later.

Anyway what it should do is something like,

  ...
  acc_address.addr_int = (acc_num + 4) & 0x7FF;
  ...
  encoded_address.d0 = 0x80 + (acc_address.addr_lo >> 2)  & 0x3F;
  ...

I'll add personally I'd use bitwise OR, "|", rather than arithmetic add, "+". In this case they should have the same effect but just my preference for bit manipulation of the sort.

As I mentioned because of the architecture difference I had to make some changes to test this on my machine, using standard types instead of old school unsigned, int, and char and I don't know if those are available with the Microchip compiler? Before making this change the "byte" part of the DCC address high byte was always giving me zero hence making the encoding incorrect ignoring any logic errors in the code. I mention this because even if the code is corrected it might be those typedefs, unions, and structs need changing to get the correct encoding?

For those that want to verify my work here's my test version of the original code,

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>


typedef union
{
  // As six address bits plus two bits that should be high for
  // a long address
  struct
  {
    uint8_t hi:6;
    uint8_t long0:1;
    uint8_t long1:1;
  };
  // as a single byte
  uint8_t byte;
} dcc_address_high;

typedef union
{
  // the two bytes
  struct
  {
    uint8_t     addr_lo;
    dcc_address_high  addr_hi;
  };
  // the whole thing as an int
  uint16_t  addr_int;
} dcc_address;

typedef struct
{
  uint8_t  d0;
  uint8_t  d1;
} dcc_encoded_address;



void
encode_dcc_address(uint16_t   event_number, bool  is_on_event)
{
  dcc_address          acc_address;
  dcc_encoded_address  encoded_address;

  // Max accessory address is 12 bit, as LS bit will be our event on/off,
  // we will use up to 11 bits of the accessory number
  acc_address.addr_int = event_number & 0x7FF;

  // Work out accessory address from event number
  // From NMRA RP9.2.1 D - accessory decoder packet format
  // First  byte is 10AAAAAA where AAAAAA is bits 3 to 8 of the accessory address
  encoded_address.d0 = 0x80 + ((((acc_address.addr_lo) >> 2) + 1) & 0x3F) + ((acc_address.addr_hi.byte << 5) & 0x20);

  // Second byte is 1AAACDDD where DDD are bits 0 to 2 of the accessory address, C is 'activate' or 'deactivate'
  // and AAA are the inverted bits 9 to 11 of the accessory address
  // For CBUS events, activate is always on, and the LSbit of the DCC address corresponds to accessory on or accessory
  // off
  encoded_address.d1 = 0x88 + (~(acc_address.addr_hi.byte << 4) & 0x70) + ((acc_address.addr_lo & 0x03)<<1 );

  if (is_on_event)
  {
    encoded_address.d1 |= 0x01; // Accessory on bit
  }

  printf("%X %X\n", encoded_address.d0, encoded_address.d1);
}


int
main(int argc, char *argv[])
{
  uint16_t  event_number = 0;

  do
  {
    encode_dcc_address(event_number, false);
    encode_dcc_address(event_number, true);

    ++event_number;
  } while (0 < event_number);

  return 0;
}

And here's my modified version,

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>


typedef union
{
  // As six address bits plus two bits that should be high for
  // a long address
  struct
  {
    uint8_t hi:6;
    uint8_t long0:1;
    uint8_t long1:1;
  };
  // as a single byte
  uint8_t byte;
} dcc_address_high;

typedef union
{
  // the two bytes
  struct
  {
    uint8_t     addr_lo;
    dcc_address_high  addr_hi;
  };
  // the whole thing as an int
  uint16_t  addr_int;
} dcc_address;

typedef struct
{
  uint8_t  d0;
  uint8_t  d1;
} dcc_encoded_address;



void
encode_dcc_address(uint16_t   event_number, bool  is_on_event)
{
  dcc_address          acc_address;
  dcc_encoded_address  encoded_address;

  // Max accessory address is 12 bit, as LS bit will be our event on/off,
  // we will use up to 11 bits of the accessory number
  acc_address.addr_int = (event_number + 4) & 0x7FF;

  // Work out accessory address from event number
  // From RailCommunity RCB-213 - Simple accessory decoder packet format
  // First  byte is 10AAAAAA where AAAAAA is bits 2 to 7 of the accessory address
  encoded_address.d0 = 0x80 | ((acc_address.addr_lo >> 2) & 0x3F);

  // Second byte is 1AAACDDR where AAA are the inverted bits 8 to 10 of the accessory address, C is 'activate'
  // or 'deactivate' (always 'activate' for CBUS events), DD are bits 0 to 1 of the accessory address, and R
  // is 0 for CBUS off events (ASOFF) or 1 for CBUS on events(ASON)
  encoded_address.d1 = 0x88 | (~(acc_address.addr_hi.byte << 4) & 0x70) | ((acc_address.addr_lo << 1) & 0x06);

  if (is_on_event)
  {
    encoded_address.d1 |= 0x01; // Accessory on bit
  }

  printf("%X %X\n", encoded_address.d0, encoded_address.d1);
}


int
main(int argc, char *argv[])
{
  uint16_t  event_number = 0;

  do
  {
    encode_dcc_address(event_number, false);
    encode_dcc_address(event_number, true);

    ++event_number;
  } while (0 < event_number);

  return 0;
}

If anyone's interested I have a spreadsheet I used to verify the output of the two versions against what was expected but it's rather large so I'll just put a relevant screen capture here,