Minor comment error fix and speed_byte mapping fix.

[bwshockley]

The first commit is a minor one in which the comment incorrectly stated the end_bit is a 0, when it is a 1.

The second commit I found to be a more robust method of assigning the speed_byte based on the 0-1023 analog input values. I found the first method was producing errors for me.

[DEGoodmanWilson]

Could you say more about the errors? I was seeing some interesting speed errors at the high end of things—directing a train to move faster than some value would produce erratic results. I have no doubt this line is specious, but I would prefer not to use map because division is slow…is there another way we can correct this using only bitshifts? If not, then map it is. I am going to meditate on this a bit.

[DEGoodmanWilson]

There is a better way to do this. Consider this snippet:

Advanced Operations Instruction (001)
The format of this instruction is 001CCCCC 0 DDDDDDDD
The 5-bit sub-instruction CCCCC allows for 32 separate Advanced Operations Sub-Instructions.
CCCCC = 11111: 128 Speed Step Control - Instruction "11111" is used to send one of 126 Digital Decoder speed steps. The subsequent single byte shall define speed and direction with bit 7 being direction ("1" is forward and "0" is reverse) and the remaining bits used to indicate speed. The most significant speed bit is bit 6. A data-byte value of U0000000 is used for stop, and a data-byte value of U0000001 is used for emergency stop. This allows up to 126 210 speed steps. When operations mode acknowledgment is enabled, receipt of a 128 Speed Step Control packet must be acknowledged with an operations mode acknowledgement.

So, the whole procedure is just wrong. For now, let's forget the speed of map, and do it correctly. Then we can optimize. Here is a proposal for modifying your PR: take it and run with it if you like!

void loop() {
  //handle reading button, controls F0
  byte button_state = digitalRead(4); //high == not pushed; low == pushed
  if(button_state && (button_state != prev_state))
  {
    //toggle!
    F0 ^= 1;
    Serial.println(F0,BIN);
    dps.setFunctions0to4(3,DCC_SHORT_ADDRESS,F0);
  }
  prev_state = button_state;

  //handle reading throttle
  analog_value = analogRead(0);
  const uint16_t dead_zone_width = 4;
  if(analog_value <= (511-dead_zone_width))
  {
    speed_byte = map(analog_value, 0, 511-dead_zone_width, -127, -2)
  }
  else if(analog_value >= (512+dead_zone_width))
  {
    speed_byte = map(analog_value, 512-dead_zone_width, 1023, 2, 127)
  }
  else
  {
    if(old_speed > 0) speed_byte = 1;
    else speed_byte = -1; 
  }

  if(speed_byte != old_speed)
  {
    Serial.print("analog = ");
    Serial.println(analog_value, DEC);
    Serial.print("digital = ");
    Serial.println(speed_byte, DEC);
    dps.setSpeed128(3,DCC_SHORT_ADDRESS,speed_byte);
    old_speed = speed_byte;
  }
  dps.update();

  ++count;
}

[bwshockley]

I will recreate the errors first and attach the output here. Then I will try the new method. I made another change that I will also post.

[bwshockley]

I made a few modifications to the original - mostly troubleshooting lighting functions, optional pins for enabling and braking the LMD18200 - not yet implemented, and finally the speed/direction. I realize in my setup, it is possible to change a loco direction with the speed still at full, not really idea, but possible. I didn't like trying to find the magic stopping point. You and I came up with the same "dead_zone" concept, just different method. I'm not a coder by trade, so I'm sure my stuff is not elegant or efficient, but it got me there. :)

#include <DCCPacket.h>
#include <DCCPacketQueue.h>
#include <DCCPacketScheduler.h>

DCCPacketScheduler dps;
unsigned int analog_value;
char speed_byte, old_speed = 0;
byte count = 0;
byte prev_state,prevDir_State = 1;
byte dir = 0;
byte lights = 0;
byte F0 = 0x01;
byte F1 = 0x02;
byte F2 = 0x04;
byte F3 = 0x08;
byte F4 = 0x10;



void setup() {
  Serial.begin(115200);
  dps.setup();

  //set up button on pin 4
  pinMode(4, INPUT);     //Light Button
  pinMode(3, INPUT);     //Direction Button
  pinMode(7, OUTPUT);    
  pinMode(8, OUTPUT);
  digitalWrite(7, HIGH); //ENALBE LMD18200
  digitalWrite(8, LOW);  //DISABLE BRAKE on LMD18200
  digitalWrite(3, HIGH); //activate built-in pull-up resistor 
  digitalWrite(4, HIGH); //activate built-in pull-up resistor  
}

void loop() {
  //handle reading button, controls F0 - F4
  byte button_state = digitalRead(4); //high == not pushed; low == pushed
  if(button_state && (button_state != prev_state))
  {
    //toggle!
    lights ^= 1;
    Serial.print("Lights = ");
    Serial.println(lights,BIN);
    if (lights == 0)
    {
      Serial.print("No lights");
      dps.setFunctions0to4(3,DCC_SHORT_ADDRESS,0);
    } else {
      Serial.print("Binary functions input = ");
      Serial.println(F0|F1|F2|F3|F4);
      dps.setFunctions0to4(3,DCC_SHORT_ADDRESS,F0|F1|F2|F3|F4);
    }
  }
  prev_state = button_state;

  //Direction
  byte direction_state = digitalRead(3); //high == not pushed; low == pushed
  if(direction_state && (direction_state != prevDir_State))
  {
    dir ^= 1;
  }
  prevDir_State = direction_state;

  //handle reading throttle
  analog_value = analogRead(0);
  speed_byte = map(analog_value, 0, 1023, 1, 127); //Remap the analog input from [0 to 1023] into [1 to 127]
  if(speed_byte != old_speed)
  {
    if(analog_value < 10) 
    {
     speed_byte = 1;
    }
    if (dir == 0)
    {
      speed_byte = speed_byte;
    } else {
      speed_byte = -1*speed_byte;
    }
    Serial.print("Direction = ");
    Serial.println(dir);
    Serial.print("analog = ");
    Serial.println(analog_value);
    Serial.print("digital = ");
    Serial.println(speed_byte, DEC);
    dps.setSpeed128(3,DCC_SHORT_ADDRESS,speed_byte);
    old_speed = speed_byte;
  }
  dps.update();

  ++count;
}

[DEGoodmanWilson]

Oh yeah, this demo code uses a single pot, with no direction switch—turn to the right, move forward, turn to the left slow, then start moving in reverse. I wanted to keep the hardware requirements dead simple.

[bwshockley]

Hmm, I like your proposal as you have a set constant for the "dead_zone". Looking into it, dead zone ends up being 9 analog values wide. I like it. I might modify it a bit to increase that to 10 values - but your method is great.

[DEGoodmanWilson]

Howdy! Did you ever update your code? Can you either push to this PR or generate a new one?

[bwshockley]

I think I should remove my "Direction" code and go back to the minimum you started with - just a single pot to control speed and direction.

Back on topic though, and the original reason I started using map instead of the bit shift was that bit shift to the right by 2 produces a speed value of 128 at 1023 which caused errors. What we need to produce is something between -127 and -2 as well as 2 to 127 for actual speeds. Based on what you wrote back on July 4 - we are going to ignore e-stops. I tweaked your code a bit, but it works well.