Skip to content
Closed
Show file tree
Hide file tree
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
70 changes: 70 additions & 0 deletions ArduinoHallEffect/ArduinoHallEffect.ino
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// An Arduino program for controlling valve timing on a 6.5ph Predator engine from Harbor Freight.
//
// Wesley Kagan, 2020
// Ric Allinson, 2020

// Control variables.
const int DEG_PER_MAGNET = 6; // Number of degrees for each magnet.
const int FREEVALVE_OFFSET_TOP = 0; // The amount of off set from TDC (needs to be multiples of DEG_PER_MAGNET).
const int FREEVALVE_OFFSET_BOTTOM = 180 + FREEVALVE_OFFSET_TOP;
Copy link

@andris-silis andris-silis Dec 15, 2020

Choose a reason for hiding this comment

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

Separate variables are necessary for intake/exhaust open/close angles (4 distinct values) as 4 stroke engine cam phases usually are not symmetrical mostly due to flow inertia (you can look them up in google - "camshaft timing chart").
For engine with free phases separate values are even more important - they will be changed by some algorithm depending on RPM and other values from sensors and lookup tables.

I guess, @Tamaren1 will come up with new code after his findings in 2nd video where he is measuring cam phases.

Copy link

@andris-silis andris-silis Dec 15, 2020

Choose a reason for hiding this comment

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

Also solenoid inertia should be coded or looked up in some table, I think. Similarly to how ECUs have injector opening delay lookup tables depending on system voltage and RPM ..
It will be fun to code as it depends on opening/closing rate and is a "real time"/not angle value :)

Not in scope of this PR though :)

Copy link
Author

Choose a reason for hiding this comment

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

Super feedback. Thanks. I've started on another branch to work in the intake/exhaust open/close. Just saw @Tamaren1 latest video. Exciting to see how the engine runs after mapping.


// Pins assignment.
const int HALL_MAGNET = 2;
const int EXHAUST_V = 12;
const int INTAKE_V = 13;

int cad; // Crank Angle Degrees (CAD).
int hallCounter; // The number of magnets after the last TDC.
bool cycle; // "true" for Intake, "false" for Exhaust.
bool printLog; // Used to trigger a print in the loop.
unsigned long timeGap; // Function level time between interrupts.
unsigned long lastTimeGap; // Global level time between interrupts.

void setup() {
Serial.begin(115200);
pinMode(HALL_MAGNET, INPUT);
attachInterrupt(0, magnetDetect, RISING);
pinMode(INTAKE_V, OUTPUT);
pinMode(EXHAUST_V, OUTPUT);
}

void loop() {
if(printLog){
Serial.print(lastTimeGap);

Choose a reason for hiding this comment

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

There should be plenty of time between interrupts but anything written out to serial should first be copied into a local variable and then written out to avoid the possibility of a race condition. Its probably not a big deal now, but could garble the output if this conditional gets interrupted.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. After thinking on it more I'm inclined to go with the current value. Removing the printLog all together. Otherwise it would mean storing all prior values until loop() ran.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, keeping the printLog variable would stop duplicates but still print the current values. I think I prefer that.

Choose a reason for hiding this comment

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

The issue isn't the printLog variable, it the variables being written out to the UART. If this part of the code is interrupted by the magnetDetect() interrupt, when the context switches back you could be in the middle of writing a variable that is now different leading to a garbled output (this would be best case scenario as you would know there was an issue). The other issue would be that its gets interrupted between Serial.print() so you could have lastGapTime be from the previous interrupt and cycle and cad from the most recent interrupt (this could go unnoticed and mess with analysis, or lead to confusion if things don't line up). I don't think any Arduino function are thread safe, so if you want to rely on the output of the log for debugging or data logging, you'll need to take some precautions such as:

// Copy to local variables
unsigned long tempTimeGap = lastTimeGap;
unsigned long tempCycle = cycle;
unsigned long tempCad = cad;

// Then write to the bus
Serial.print(tempTimeGap);
Serial.print(tempCycle);
Serial.print(cad);

Again, probably not going to be a big deal until you start dumping lots of data out onto the bus or lots of other things start happening in the loop() function.

Copy link
Author

Choose a reason for hiding this comment

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

It seem declaring them asvolatile forces a lookup negating the garbled output. It doesn't help with the second half of the issue of having values from two different interrupts. But I can't see how that could be avoided even with tempVars unless you keep a temporal list for the last n?

Choose a reason for hiding this comment

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

Its all a trade off, copying everything to a temporary variable is fast so you don't have to worry about getting everything out before the interrupt triggers. Ideally you'd be writing into one side of a log buffer and dumping it to the UART from the other side. The tempVars won't change even if the loop() is interrupted:

// Copy to local variables
unsigned long tempTimeGap = lastTimeGap;
unsigned long tempCycle = cycle;
unsigned long tempCad = cad;

// Then write to the bus
Serial.print(tempTimeGap);
---------> INTERRUPT TRIGGERS HERE
lastTimeGap, cycle, cad, printLog are updated
<-------- INTERRUPT RETURNS, CONTEXT SWITCHES BACK TO HERE
Serial.print(tempCycle); <------tempCycle still has the previous cycle value
Serial.print(tempCad); <------ tempCad still has the previous cad value

printLog = false;

To avoid the issue of a missed block when the interrupt happens in the middle of the loop(), you can move the printLog = false; to the beginning of the loop. That way if the interrupt hits somewhere in the middle, the new data will be spit out on the next pass.

Serial.print(cycle);
Serial.print(cad);
printLog = false;
}
}

// This function will be called about every 15.79ms at the maximum RPM of 3800.
void magnetDetect() {
// Increment the counter to keep track of the position.
hallCounter++;
// Get the time gap between this interrupt and the last.
timeGap = millis() - lastTimeGap;

// Find missing tooth for Top Dead Center (TDC).
if (timeGap >= lastTimeGap * 3 / 2) {
// Reset the hall counter.
hallCounter = 1;
// Flip the cycle phase.
cycle = !cycle;
}

// Store the last time difference so we can use it in the next interrupt.
lastTimeGap = timeGap;

// Store the current crank angle for logging.
cad = hallCounter * DEG_PER_MAGNET;

// Every rotation of the crank alternates between Intake and Exhaust.
if (cycle) {
// If the crank angle degree is in the intake range open it, otherwise close it.
digitalWrite(INTAKE_V, cad > FREEVALVE_OFFSET_TOP && cad <= FREEVALVE_OFFSET_BOTTOM);
} else {
// If the crank angle degree is in the exhaust range open it, otherwise close it.
digitalWrite(EXHAUST_V, cad >= FREEVALVE_OFFSET_BOTTOM || cad < FREEVALVE_OFFSET_TOP);
}
printLog = true;
}
71 changes: 0 additions & 71 deletions ArduinoHallEffect_good_code.ino

This file was deleted.