Proper programming processes suggestions

Purpose:
I know it is long, but I did not know how to present it better.
As someone new to programming and C++ in particular and my experience is from 45 or so years ago in interpretive basic with a TRS 80. I have forgotten more than I ever knew by now. Although I had an idea of a. what I wanted to accompliish and b. a general process in which to accomplish it. I am unfamiliar with the finer ways of doing things.
Below is a description of what I am doing, and included are two questions related to the project.
Finally there is the complete code of the transmitter side of the project. I am in the process of completing the receiver side now. The transmitter essentially does everything I originally wanted it to.

Determine the current position of a rotating ring by 4 reed switches in a BCD arrangementand a hall effect sensor triggering an interrupt. The 4 switches create a BCD value (1 to 8) which is calculated within the ISR. Additionally the direction of rotation (CW/CCW) is determiined and if rotation reaches 405 degrees makes motor enable false.

Upon determining the above parameters, the ISR returns to the void (loop) If motor enable is false the motor enable will remain false for about .5 seconds then return to true. (disabling the motor automatically reverses the direction of rotation.).

CW adds 16 to the BCD value, CCW adds 0 to it. The total value is transmitted to a receiver approximately 10-11 meters away.

The Receiver will decode the received value a 8 bit integer and convert the 1 - 8 value to points on a compass (every 45 degrees, N, NE, E, SE, S, SW, W, & NW ) as well as determine whether the CW or CCW rotation is occuring (=> 16). This information is sent to an OLED display for the operator to see what is happening and make decisions as to go or stop.

Just a point: there is a cable which is attached to the rotating ring, which is why the traval is limiter, also just in case it is not clear, the ring can go either direction CW or CCW.

This a roughly working project with the first section working fine determing current position and direction. The second part(the receiver) is waiting the OLED and testing of range. Both transmitter and receiver have antenna's attached. (Serial.print appears to present the right data so I think I only need to get the display working to finish up the receiver section.

As I think about it, there are several questions I have that someone may be able to answer or make suggestions.

  1. If my rotation say, goes from 4 towards 5, but is reversed prior to getting to 5 then the next trigger will be 4 again. I use previous value = to actual value at the end of the ISR, Since now both values will become 4, I no longer know the rotational direction. I am thinking if two triggerd values are equal, then the direction should just be inverted. Other suggestions?

  2. I was told early on that for something as slow as this perhaps 5-8 seconds between interrupts, that I should not use interrupts but do the processing that is done in the ISR in the main program. I was told that sometime in the future an interrupt will bite me, even it was working fine up to then. Since almost everything in the transmitter side was working at the time, I did not want to be changing something that worked until I could get all of reliabley functioning. I am at that point I think, but have no idea as to how to move the funcionality of what is in the ISR to a different area. I have only been programing arduino for 4-5 weeks and it has been a climb to learn what I have learned, but some ways of doing thigs elude me at this time.

In summary, I am not necessarily looking for specifics but rather good programming ways to accomplish a good polished sketch.

Thank you in advance for help provided.

A lot of the Serial.prints will be removed as I used them for testing, I also will not be using a desplay in the transmitter so the library LiquidCrystal.h should also be removed.

/*
   Logic program to determine rotation direction and direction pointing
   Rotation CW will be represented as  value of 16 of the uint8_t
   which will include the least significant bits 0-3 for values between
   1 and 8 only.  To indicate rotation direction if value is greater than
   8 then CW else CCW.  Values between 9 and 15 are invalid as is 0.
*/

#include <VirtualWire.h>
#include <LiquidCrystal.h>

const uint8_t trigger = 2;   // the number of the interrupt trigger pin
const uint8_t   reed1 = 3;   // Reed group pins 4-7 are BCD inputs
const uint8_t   reed2 = 4;   //
const uint8_t   reed3 = 5;   //
const uint8_t   reed4 = 6;   //
const uint8_t  rotate =11;   // detects call to rotate antenna from base
const int ledPin = 13;
uint8_t i = 0;
uint8_t triggered = 0;
uint8_t TX_buffer[1] = {0};
const uint8_t NUMBER = 4;                   // Array of inputs for BCD reading
uint8_t input_pin[NUMBER] = {3, 4, 5, 6};
uint8_t position[NUMBER] = {0};
bool runonce = false;
String cw_ccw = " CW";
uint8_t comvalue= 1;


// variables will change:
volatile uint8_t value = 0;
volatile uint8_t triggerState = 0;
volatile uint8_t prev_value = 0;
volatile uint8_t mot_ena = 1;         // enables motor thru MOSFET
volatile uint8_t cw = 1;


void setup()
{
  Serial.begin(115200);
  pinMode   (trigger, INPUT);
  pinMode   (rotate,  INPUT);
  pinMode   (trigger, INPUT);     // Interrupt input from Hall effect
  pinMode   (ledPin, OUTPUT);
  vw_set_tx_pin(12);              // Transmit pin base data for direction and rotation
  vw_setup(2000);                 // bps transmission speed of transmitter
  TX_buffer[0] = 0;
  
  attachInterrupt(digitalPinToInterrupt(trigger), conv, FALLING);
}

void loop()
{
if (mot_ena = 0)       
  {
    digitalWrite (mot_ena, 0);
    delay (5000);
  
    digitalWrite (mot_ena, 1);
  } 
 delay(20);
 if (cw_ccw == "CW") {
  comvalue = (value + 16);
 } else {
  comvalue = value;
 }
 while (triggerState == 1)
 {
    // **************cw is not being evaluated for here
  
  TX_buffer[0] = (comvalue);
 
    vw_send(TX_buffer,1);
    vw_wait_tx();

 Serial.print ("comvalue");
 Serial.print ("\t");    
 Serial.println (comvalue);
   triggerState = 0;

 delay (50);
 digitalWrite(ledPin, 1);
/*   Serial.print("Triggered Value:");
  Serial.print("\t");
  Serial.println(value);*/
  } 
  
} 

// ISR conv Converts inputs to integer reading reed switches 1,2,4,8. Triggered on hall effect input on interupt pin.

void conv()
{
        // Current state should be equal to the 4 input BCD values.
  triggerState = 1;
  Serial.print("triggerState2:   ");  // Testing only (2 is only an indicator of 
  Serial.println(triggerState);       // Testing only where it was printed from)
    value = 0;   //Resets value to zero, stops accumulation of value growing
    position[1] = digitalRead(reed1);
    value = value * 2 + position [1];
    //digitalWrite (reed_1, position [1]);
    position[2] = digitalRead(reed2);
    value = value + position[2] * 2;
    //digitalWrite (reed_2, position [2]);
    position[3] = digitalRead(reed3);
    value = value + position[3] * 4;
    // digitalWrite (reed_3, position [4]);
    position[4] = digitalRead(reed4);
    value = value + position[4] * 8;
    //digitalWrite (reed_4, position [8]);

    // Determines rotation direction
    if (value >= prev_value)
    { 
      cw = 1;
      cw_ccw = " CW";
    } 
    else 
    {
      cw = 0;
      cw_ccw = "CCW";
    }
      Serial.print ("cw");
      Serial.print ("\t");    
      Serial.println (cw);
    
    // determines if rotation went too far in either direction
    if (((value == 1) & (prev_value == 8)) & (cw = 1) || ((value == 7) & (prev_value == 8) & (cw = 0)))
    {
      mot_ena = 0;
    } 
    else 
    {
      mot_ena = 1;
    }

    Serial.print ("current value: ");
    Serial.print ("\t");
    Serial.println(value);
    Serial.print ("rotation: ");
    Serial.print ("\t");
    Serial.println(cw_ccw);
    Serial.print ("previous value: ");
    Serial.print ("\t");
    Serial.println(prev_value);
    Serial.println("++++++++++++++++++");
  prev_value = value;
  digitalWrite (mot_ena, HIGH);
  delay(25);
}

Too much to read.
:pleading_face:


Please draw a diagram of the layout.


Show us a good schematic of your circuit.
Show us a good image of your β€˜actual’ wiring.
Give links to components.


Not a good start:
if (mot_ena = 0)
= is not the same as ==
== is for comparison
= is for assignment

Serial.print will not work inside an ISR.

Make ISRs short.

β€œ attachInterrupt(digitalPinToInterrupt(trigger), conv, FALLING);”
Interrupts are generally used for very fast signals that need immediate attention.
Slow events are best handled with polling.

Why two trigger ?

Your array only has 4 elements, you access them with indices 0...3, not 1...4

Your ISR should only read the reed switches and set a flag. You main loop() function should do all the other calculations when that flag is set.

interrupts are off inside your ISR so delay() will not work, nor will Serial.print() since both rely on interrupts to function.

You are confusing your mot_ena value with whatever pin controls that motor (rotate?) I would suggest always appending "Pin" to const variables that represent pins to avoid confusion.
This might get you going.... (untested)

/*
   Logic program to determine rotation direction and direction pointing
   Rotation CW will be represented as  value of 16 of the uint8_t
   which will include the least significant bits 0-3 for values between
   1 and 8 only.  To indicate rotation direction if value is greater than
   8 then CW else CCW.  Values between 9 and 15 are invalid as is 0.
*/

#include <VirtualWire.h>
#include <LiquidCrystal.h>

const uint8_t triggerPin = 2;   // the number of the interrupt trigger pin
const uint8_t  rotatePin = 11;  // detects call to rotate antenna from base
const int ledPin = 13;
//uint8_t i = 0;
//volatile bool triggered = false

//uint8_t TX_buffer[1] = {0};

const uint8_t NUMBER = 4;                   // Array of inputs for BCD reading
const uint8_t inputPin[NUMBER] = { 6, 5, 4, 3 };   // MSB to LSB {3, 4, 5, 6};
//uint8_t position[NUMBER] = {0};
//bool runonce = false;



// variables will change:
volatile uint8_t value = 0;
volatile uint8_t prev_value;
volatile bool triggerState = false;
//volatile uint8_t prev_value = 0;
volatile bool mot_ena = true;         // enables motor thru MOSFET

enum { CCW, CW };
//String cw_ccw = " CW";
volatile uint8_t direction = CW;

void setup()
{
  Serial.begin(115200);
  pinMode(triggerPin, INPUT);
  pinMode(rotatePin,  OUTPUT);
  pinMode(ledPin, OUTPUT);
  for ( int i = 0; i < NUMBER; ++i ) {
    pinMode( inputPin[i], INPUT );
  }
  vw_set_tx_pin(12);              // Transmit pin base data for direction and rotation
  vw_setup(2000);                 // bps transmission speed of transmitter
  //  TX_buffer[0] = 0;

  attachInterrupt(digitalPinToInterrupt(triggerPin), conv, FALLING);
}

void loop()
{
  uint8_t comValue = value;

  if (mot_ena == false) {
    digitalWrite (rotatePin, LOW);
    delay (5000);
    mot_ena = true;
  }

  if ( mot_ena ) {
    digitalWrite (rotatePin, HIGH);
  }

  if (direction == CW) {
    comValue = value + 16;
  }

  if (triggerState == true) {
    calculate();
    // **************cw is not being evaluated for here

    vw_send(&comValue, sizeof(comValue));
    vw_wait_tx();

    Serial.print ("comValue");
    Serial.print ("\t");
    Serial.println (comValue);
    triggerState = false;

    delay (50);
    digitalWrite(ledPin, HIGH);
  }

}

// ISR conv Converts inputs to integer reading reed switches 1,2,4,8. Triggered on hall effect input on interupt pin.

void conv()
{

  // Current state should be equal to the 4 input BCD values.
  triggerState = true;
  value = 0;   //Resets value to zero, stops accumulation of value growing
  for ( int i = 0; i < NUMBER; ++i ) {
    value = value * 2 + digitalRead( inputPin[i] );
  }

  // Determines rotation direction
  if (value >= prev_value) {
    direction = CW;
  }
  else {
    direction = CCW;
  }
  // determines if rotation went too far in either direction
  if (
    ((value == 1) and (prev_value == 8) and (direction = CW))
    or ((value == 7) and (prev_value == 8) and (direction = CCW))
  ) {
    mot_ena = false;
  }
  else {
    mot_ena = true;
  }
  prev_value = value;
}

void calculate()
{
  Serial.print("triggerState2:   ");  // Testing only (2 is only an indicator of
  Serial.println(triggerState);       // Testing only where it was printed from)
  Serial.print ("direction:\t");
  if ( direction == CCW ) {
    Serial.println("CCW");
  }
  else {
    Serial.println("CW");
  }
  Serial.print ("current value:\t");
  Serial.println(value);
  Serial.print ("previous value:\t");
  Serial.println(prev_value);
  Serial.println("++++++++++++++++++");
}

Very helpful. The mot_ena does not directly control the motor power for the motor comes from a switch external to the transmitter end, mot_ena only enables the MOSFET and disables it for a half-second if travel is too far to reverse motor direction. Normally on.

But I see that I do need to ascribe a pin to it so I can connect it to the MOSFET
Thanks you

My error, and it did not fault during compile so I missed it.

Yes, I missed that one, I should have caught it because I know better. ==

triggerState = 1;
Serial.print("triggerState2: "); // Testing only (2 is only an indicator of
Serial.println(triggerState); // Testing only where it was printed from)
triggerState2 is in quotes just an indicator so I knew what part of the program it was coming from, as I was printing the status of triggerState in several locations.
Serial.print does in fact print within the ISR, of course I did not know it was not supposed to so I had it in there and it prints exactly as I have it:
triggerState2: 1
current value: 8
rotation xx: CW
previous value: 0
++++++++++++++++++
I added the xx to rotation to make sure it was within the ISR

I need to figure out polling so I can move it all of it out of the ISR.
Thank you for your input, helps a great deal.

Missed that one I should have put in == and I know better.

I will provide the circuit diagram. Currently the system works as follows:
power on in base unit, press left or right switch and unit rotates as long as switch is held down, No Feedback. This is only to provide feedback and prevent power cable from wrapping around post by reversing direction is if goes 45 deg. past the start point fixed at value 1, (Too Far) depending on CW or CCW. Power goes up the cable to the motor, I install a MOSFET in between so I can interrupt the power if it goes to far. Motor automatically reverses on loss of power, so I kill it for .5 seconds. Diagram should tell the rest, give me some time to make it
woodcrafter

Suggest you simply scan the inputs every 20ms or so.


I hope this gives you a better idea of the function of this sketch. I will start cleaning it up removing Screen.prints that are not necessary or at least commenting them out so in case I have to revisit I will have some T/S there to help. I will create a seperate sketch to see if I can make polling work, I am convinced that it has to stay in sync with the trigger hall effect and only use the position value when the hall effect is triggered.
I am concerned placement and triggering points of the reed switches will not all be the same and I will get a false reading. I guess I will need more code to determine if the value read is the expected value based on the last reading and anticipated direction of rotation. Motor rotation is not precise and not easy to manage, can't easily sweep back and forth searching for the correct position. Got some testing to do for sure.

Here is an example of scanning switches and other sensors every 20ms.

//TIMER examples

#include <Servo.h>
Servo myservo;

#define enabled                  true
#define disabled                 false

#define PUSHED                   LOW
#define notPUSHED                HIGH


const byte heartbeatLED        = 13;
const byte startSwitch         = 2;     //+5V---[50k INPUT_PULLUP]---[switch]---GND
const byte servoPin            = 3;

bool servoFlag                 = disabled;

byte lastStartSwitchStatus;

//**********************************
//timing stuff
unsigned long servoMillis;
unsigned long heartbeatMillis;
unsigned long switchMillis;


//***************************************************************************
void setup()
{
  pinMode(startSwitch, INPUT_PULLUP);

  pinMode(heartbeatLED, OUTPUT);

  myservo.attach(servoPin);
  myservo.write(0);

} //END of setup()


//***************************************************************************
void loop()
{
  //*********************** 1/2 second TIMER
  //time to toggle the heartbeatLED ?
  if (millis() - heartbeatMillis >= 500)
  {
    //restart the TIMER
    heartbeatMillis = millis();

    //toggle the LED
    digitalWrite(heartbeatLED, !digitalRead(heartbeatLED));
  }

  //*********************** 20ms TIMER
  //time to check the switches and sensors ?
  if (millis() - switchMillis >= 20)
  {
    //restart the TIMER
    switchMillis = millis();

    checkSwitches();
  }

  //***********************  2 second TIMER
  //if we are timing, has the 2 seconds servo TIMER expired ?
  if (servoFlag == enabled && millis() - servoMillis >= 2000)
  {
    servoFlag = disabled;

    //move the servo to 0'
    myservo.write(0);

  }


  //*********************************
  //Other non blocking code goes here
  //*********************************

} //END of loop()


//***************************************************************************
//the following switches and sensors will be tested every 20ms
void checkSwitches()
{
  byte switchStatus;

  //****************************************
  //start switch
  switchStatus = digitalRead(startSwitch);

  //**********************
  //was there a change in switch state ?
  if (lastStartSwitchStatus != switchStatus)
  {
    //update to the new state
    lastStartSwitchStatus = switchStatus;

    //*************
    if (switchStatus == PUSHED)
    {
      //move the servo to 180'
      myservo.write(180);

      //restart the servo TIMER
      servoMillis = millis();

      //enable the servo TIMER
      servoFlag = enabled;

    }

  } //END of  if (lastStartSwitchStatus != switchStatus)


  //****************************************
  //other switch and sensor code
  //****************************************

} //END of checkSwitches()


//***************************************************************************

Thank You LarryD, helps me to understand the process. I could not get it in my head as to how to do that. I quickly looked over your example, I will have to study it in more detail.
I 3D printed the cylinder which will house the magnets and hopefully they were printed properly aligned. Looks like it. I put the read switches horizontal, hoping that would help them all trigger by the time the hall effect triggers the process.
I think I can test for the hall effect and if triggered, read the other switches otherwise keep polling until I do get a read of the hall effect.

Having a difficult time visualizing the setup.

Can you take a few pictures ?

Having a difficult time visualizing the setup. Can you take a few pictures? I can imagine, it is simple but you got to see it to fully understand the process.
ok, the cylinder will fit on the antenna base and will rotate with the antenna, motor is enclosed in the center section cylinder fits around the outside ring. The switches are on a stationary stick that fits on the mount for the pole, not shown, and face the magnets that will be in the cylinder where you see the pockets according to BCD setup 1-8 and of course the trigger ring every 45 degrees. Hope this helps.
Been under consideration for two years but only recently did I decide to learn to code and make it a reality. My background is in electronics and IT hardware, so it is more of a stretch to begin learning this to make it happen. I really do appreciate your input and I am working on your example, I think I can make it work.
BTW I have a reed switch where the hall effect transistor will be placed, change my idea after I made the switch holder.

I could have purchased an antenna that does all I am working on, but then I would not know what I do now.

Good 3D cylinder.

Are those 5 reed limit switches ?

40 magnets ?

Currently I an just changing the input and triggering the hall effect with a magnet for testing, will assemble after I am satisfied that all works, then the challenge of making the mechanical work too, like positioning of the parts in relation to each other. I
haven't tested the range of the communications, need about 15 or 20 feet reliable communications to work.

A good review about reed switches: