Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 48 additions & 57 deletions ArduinoHallEffect_good_code.ino
Original file line number Diff line number Diff line change
@@ -1,71 +1,62 @@
//This code is full of test strings and is generally a conglomerate of code from stackexchange and
//bad form. If it looks stupid, I probably wrote it.
//bad form. If it looks stupid, I probably wrote it.
//Wesley Kagan, 2020

//this might be of interest https://forum.arduino.cc/index.php?topic=4324.0 fast pin switching at 2MHz on Uno

const int hall = 2;
const int INTAKE_V = 13;
const int EXHAUST_V = 12;
int hallcounter = 0;
int hallstate = 0;
int lasthallstate = 0;
unsigned long triggerTime;
unsigned long last_triggerTime;
unsigned long timeGap;
unsigned long last_timeGap;
unsigned int degree;
unsigned int state;
int newstate;

int degree;
int cycle = 1; //1 is intake cycle, -1 is exhaust, change initial cycle here

void setup() {
pinMode(hall, INPUT);
Serial.begin(115200);
pinMode(13, OUTPUT); // sets the digital pin 13 as output
pinMode(12, OUTPUT); // sets the digital pin 12 as output
attachInterrupt(0, magnet_detect, RISING);//Initialize the intterrupt pin digital pin 2
degree = 0;
pinMode(hall, INPUT);
Serial.begin(115200);
pinMode(INTAKE_V, OUTPUT); // sets the digital pin 13 as output
pinMode(EXHAUST_V, OUTPUT); // sets the digital pin 12 as output
attachInterrupt(0, magnet_detect, RISING); //Initialize the intterrupt pin digital pin 2
degree = 0;
}

void loop()
{
//The compiler says this is needed otherwise the govt. takes my cat
}
void loop() {
//The compiler says this is needed otherwise the govt. takes my cat
}

void magnet_detect() {
hallcounter ++;
triggerTime = millis();
timeGap = triggerTime - last_triggerTime;
last_triggerTime = triggerTime;

if (timeGap >= last_timeGap + last_timeGap / 2)
{
//Serial.println(state);
Serial.println("missing tooth");
hallcounter = 1;
state++; //This right here is garbage and you know it.
hallcounter++;
timeGap = millis() - timeGap;

if (timeGap >= (int)(1.5 * last_timeGap)) {
Copy link

@ricallinson ricallinson Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand why this is cast to an (int) here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably to avoid a casting for timeGap to a float. Only numbers of the same type can be compared.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks. It should probably be a (long) to keep it consistent with the type declaration of these variables.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider multiplying the timeGap by 10 and last_timeGap by 15 to prevent calling into the floating point libraries. See this: https://github.com/nkapron/Freevalve_Arduino/blob/054797a65dec251340a225ef7532224851ff92c9/valve_control.ino#L142

Copy link
Author

@jrsall92 jrsall92 Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider multiplying the timeGap by 10 and last_timeGap by 15 to prevent calling into the floating point libraries. See this: https://github.com/nkapron/Freevalve_Arduino/blob/054797a65dec251340a225ef7532224851ff92c9/valve_control.ino#L142

Actually I like this approach quite a lot!

@ricallison floats are not "convinient" numbers to work on in computers, they don't exactly have the value you think they have, also conversion will happen automatically to compare an int to a float, as such losing accuracy. My way of casting the float to int basically turncates the float number to decimals only, i.e. 14.7 => 14 and 14.3 => 14, which was ok since the comparison was for larger, but the multiplication method is better and more accurate, even if potentialy a smige slower because now we have to do 2 multiplications, instead of a casting

//Serial.println(state);
Serial.println("missing tooth");
hallcounter = 1;
cycle = -1 * cycle;
}
last_timeGap = timeGap;

//Serial.println(hallcounter);
degree = hallcounter * 6;
if (cycle > 0) { //Bool wasn't working so this dumpster fire got started.
Serial.print("STATE 1 ");
if (degree <= 180) { //EDIT HERE INTAKE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment on PR #2 - a single degree value conditional per "cam" is not enough - there should be a range and it can also overlap between phases/cycles https://github.com/Tamaren1/Freevalve_Arduino/pull/2/files#r543460537

digitalWrite(INTAKE_V, HIGH); // sets the digital pin 13 on
}

last_timeGap = timeGap;
//Serial.println(hallcounter);

degree = hallcounter * 6;
if ((state % 2) == 0) { //Bool wasn't working so this dumpster fire got started.
Serial.print("STATE 1 ");
Serial.println(degree);
if (6 <= degree && degree <= 180) { //EDIT HERE INTAKE
digitalWrite(13, HIGH); // sets the digital pin 13 on
}
else {
digitalWrite(13, LOW); // sets the digital pin 13 off
}
}
if ((state % 2) == 1) {
Serial.print("STATE 2 ");
Serial.println(degree);
if (180 <= degree && degree <= 354) { //EDIT HERE EXHAUST
digitalWrite(12, HIGH); // sets the digital pin 13 on
}
else {
digitalWrite(12, LOW); // sets the digital pin 13 off
}
}
lasthallstate = hallstate;
}
else {
digitalWrite(INTAKE_V, LOW); // sets the digital pin 13 off
}
}
else {
Serial.print("STATE 2 ");
if (180 <= degree) { //EDIT HERE EXHAUST
digitalWrite(EXHAUST_V, HIGH); // sets the digital pin 12 on
}
else {
digitalWrite(EXHAUST_V, LOW); // sets the digital pin 12 off
}
}
Serial.println(degree);
}