Hello, I’m working on a Camless Petrol Engine project and I’m using the following rotary encoder: https://www.amazon.fr/gp/aw/d/B015GYY7XU?psc=1&ref=ppx_pop_mob_b_asin_title
as a crankshaft position sensor. When the sensor is turned a bit faster, its values jump sometimes by increments of 40. I’d like to know if there’s a coding solution to this problem. Thank you !
Possibly... it would help if you showed the code you are currently using, and the values you are getting.
Sure, here's the code
volatile unsigned int temp, counter = 0; //This variable will increase or decrease depending on the rotation of encoder
void setup() {
Serial.begin (9600);
pinMode(2, INPUT_PULLUP); // internal pullup input pin 2
pinMode(3, INPUT_PULLUP); // internalเป็น pullup input pin 3
pinMode(10, OUTPUT);
pinMode(11, OUTPUT);
//Setting up interrupt
//A rising pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
attachInterrupt(0, ai0, RISING);
//B rising pulse from encodenren activated ai1(). AttachInterrupt 1 is DigitalPin nr 3 on moust Arduino.
attachInterrupt(1, ai1, RISING);
digitalWrite(10, HIGH);
digitalWrite(11, HIGH);
}
bool inRange(int minimum, int maximum, int val) {
return ((minimum<=val) && (val<= maximum));
}
void loop() {
// Send the value of counter
if( counter != temp ){
Serial.println (counter);
temp = counter;
}
if(inRange( 0, 40, counter)){
digitalWrite(10, LOW);
}
if( inRange( 560, 600, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 1760, 1800, counter)){
digitalWrite(11, LOW);
}
if(inRange( 2340, 2380, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 2381, 2410, counter)){
digitalWrite(10, LOW);
}
if(inRange( 2930, 2970, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 4130, 4170, counter)){
digitalWrite(11, LOW);
}
if(inRange( 4710, 4750, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 4751, 4785, counter)){
digitalWrite(10, LOW);
}
if(inRange( 5300, 5345, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 6490, 6540, counter)){
digitalWrite(11, LOW);
}
if(inRange( 7075, 7115, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 7116, 7145, counter)){
digitalWrite(10, LOW);
}
if(inRange( 7670, 8115, counter)){
digitalWrite(10,HIGH);
}
if(inRange( 8870, 9010, counter)){
digitalWrite(11, LOW);
}
if(inRange( 9440, 9495, counter)){
digitalWrite(11,HIGH);
}
if(inRange( 9495, 9540, counter)){
digitalWrite(10, LOW);
}
if(inRange( 10020, 10085, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 11235, 11285, counter)){
digitalWrite(11,LOW);
}
if(inRange( 11810, 11865, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 11865, 11920, counter)){
digitalWrite(10, LOW);
}
if(inRange( 12405, 12465, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 13605, 136465, counter)){
digitalWrite(11, LOW);
}
if(inRange( 14160, 14215, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 14218, 14260, counter)){
digitalWrite(10, LOW);
}
if(inRange( 14770, 14835, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 15970, 16035, counter)){
digitalWrite(11, LOW);
}
if(inRange( 16550, 16590, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 16590, 16625, counter)){
digitalWrite(10, LOW);
}
if(inRange( 17140, 17205, counter)){
digitalWrite(10, HIGH);
}
if(inRange( 18340, 18410, counter)){
digitalWrite(11, LOW);
}
if(inRange( 18915, 18975, counter)){
digitalWrite(11, HIGH);
}
if(inRange( 18976, 19025, counter)){
digitalWrite(10, LOW);
}
if(inRange( 19510, 19610, counter)){
digitalWrite(10, HIGH);
}
}
void ai0() {
// ai0 is activated if DigitalPin nr 2 is going from LOW to HIGH
// Check pin 3 to determine the direction
if(digitalRead(3)==LOW) {
counter++;
}else{
counter--;
}
}
void ai1() {
// ai0 is activated if DigitalPin nr 3 is going from LOW to HIGH
// Check with pin 2 to determine the direction
if(digitalRead(2)==LOW) {
counter--;
}else{
counter++;
}
}
And the values I have been getting:
9816
9822
9829
9836
9844
9851
9859
9867
9874
9882
9890
9898
9905
9912
That device appears to be an incremental encoder. I'd have thought that an absolute encoder would be appropriate here if you are replacing the camshaft (and presumably valve control) with electronics.
You may get glitches on an 8 bit arduino if you set a 16bit variable in an interrupt service routine and use it directly in the loop() without first making a copy of it while interrupts are suspended. (Topic: Non atomic read operations)
Try commenting out all the if statements which control the leds to see if you get better results.
If pullup resistors are appropriate, you may get better results with falling interrupts.
Edit
Also try increasing the baud rate from 9600 to 115200.
Also check your assumption that you should see the values increment cleanly by units of 1 during acceleration. If the acceleration is slow you may even see the same values repeated.
There WILL be glitches. Here is how to protect counter from being corrupted by the interrupt, while being accessed by main:
noInterrupts(); //turn off interrupts
int counter_copy = counter; //make a copy
interrupts(); //back on
...
Serial.println(counter_copy); // use only the copy in the main loop
That may not be the only problem you have, but it is one of them.
The normal way to use a rotary encoder is to use a state machine, this will be self correcting of glitches and contact bounce. Being on a distributor you will only need to count in one direction.
How can I program a state machine with the material I have ?
I can’t manage it in my code, sorry but can you show a concrete exemple of this code usage ?
Alright I figured it out.
The problem was due to the fact that the serial port communication rate being too slow (9600baud), it consequently affected my project since my encoder is used here as a distributor: in the loop, the “SerialPrintIn” stands before all the angle verifications for the leds; meaning that it has first to send the reading to the PC via serial port before even verifying that it can distribute power to a LED or not. That operation of sending being in itself slow and being slowed down by the rate of 9600, it was jumping values and therefore could not trigger the LEDs at the right time.
This crossed with your "solved" message. . .
Following the product link in the OP, a review of that optical encoder can be found which contains an oscilloscope picture of the device in action:
- I would guess that 4.7k or maybe 2.2k external pullup resistors would give an even cleaner picture.
- Bouncing does not appear to be a problem (it is anyway an optical encoder - no mechanical contacts).
- Trigger the interrupts on the falling edges, not rising.
Can you say a bit more about the mechanics. A Camless Petrol Engine project (I guess 4 stroke) sounds interesting.
There is a big potential to make those if statements which control the leds much more efficient.
Ok, understood I'll try interrupting on the falling edges. On this encoder, a complete rotation means 1200 points.
I'm working on a 4 stroke Engine, the valves are driven by two 12V solenoids, commanded by a relay board that is connected to pin 10 and 11 on the Arduino board.
I'm working right now on the "if" statements: When the angle value is at point 0, I set the first actuator open (digitalWrite 10, LOW), this is my admission phase. When a half rotation (value 600) is completed ( meaning the piston has gone from top dead center to bottom dead center ), I close the admission valve and then starts the compression phase. Then for the exhaust phase, when one and a half rotation is done ( value 1800 ), I open the exhaust valve and close it at somewhere near 2395. What I would like to do now is to reset the counter at 0 when the value of 2400 is reached, meaning that the crankshaft has done two complete rotations, in order to reset the cycle. I've tried the following solution: if(inRange(2396, 2400, counter)){ counter, temp=0; }
but it didn't reset the counter value to 0, if you have any solutions
Relays to drive solenoids? You do know that relays contain solenoids to move their switch contacts? This seems like it would make getting the timing correct quite difficult. You would need to consider the switching delay of the relays as well as the movement delay of the solenoids. Plus, those relays may wear out pretty fast! Perhaps you should consider MOSFETs to control the solenoids instead of relays. For example IRLZ44. The switching time of a MOSFET should be insignificant compared to a relay.
It is true I haven't thought about that, I'll try to take into account the delay in my angle values for tuning but I might consider buying mosfets to get it faster.
Can you guys help me in the program to reset the counter value to 0 when 2 rotations are completed on the shaft, here's the code in its current state:
volatile unsigned int temp, counter, counter_copy = 0; //This variable will increase or decrease depending on the rotation of encoder
void setup() {
Serial.begin (2000000);
pinMode(2, INPUT_PULLUP); // internal pullup input pin 2
pinMode(3, INPUT_PULLUP); // internalเป็น pullup input pin 3
pinMode(10, OUTPUT);
pinMode(11, OUTPUT);
//Setting up interrupt
//A falling edge pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
attachInterrupt(0, ai0, FALLING);
//B falling edge pulse from encodenren activated ai1(). AttachInterrupt 1 is DigitalPin nr 3 on moust Arduino.
attachInterrupt(1, ai1, FALLING);
digitalWrite(10, HIGH);
digitalWrite(11, HIGH);
}
bool inRange(int minimum, int maximum, int val) {
return ((minimum<=val) && (val<= maximum));
}
void loop() {
if(inRange( 0, 40, counter)){
digitalWrite(10, LOW);
}
if(inRange(580, 620, counter)){
digitalWrite(10, HIGH);
}
if(inRange(1780, 1820, counter)){
digitalWrite(11, LOW);
}
if(inRange(2365, 2395, counter)){
digitalWrite(11, HIGH);
}
if(inRange(2396, 2400, counter)){
counter, temp=0;
}
if( counter != temp ){
Serial.println (counter_copy);
temp = counter_copy;
}
}
void ai0() {
// ai0 is activated if DigitalPin nr 2 is going from LOW to HIGH
// Check pin 3 to determine the direction
if(inRange(2396, 2400, counter)){
counter, temp = 0, 0;
}
if(digitalRead(3)==HIGH) {
counter++;
noInterrupts();
int counter_copy = counter;
}
if(digitalRead(3)==LOW){
counter--;
noInterrupts();
counter_copy=counter;
}
}
void ai1() {
// ai0 is activated if DigitalPin nr 3 is going from LOW to HIGH
// Check with pin 2 to determine the direction
if(inRange(2396, 2400, counter)){
counter, temp = 0, 0;
}
if(digitalRead(2)==HIGH) {
counter--;
noInterrupts();
int counter_copy = counter;
}
if(digitalRead(2)==LOW){
counter++;
noInterrupts();
counter_copy = counter;
}
}
In principle, when the variable counter reaches 2399 you want to reset it to 0.
However, the encoder code you are basing your solution on appears to be too heavyweight for this application, assuming the motor always runs in the same direction. You never have to consider the motor running in reverse or do you ?
I guess you have a general problem with using an incremental encoder instead of an absolute encoder, as I mentioned earlier. At system start, there is no reference between the physical state of the motor and the state your program maintains of the encoder.
If it is only a proof of concept type project, I suppose you could have a single sensor dedicated to detecting top dead center (TDC) and, at the start, rotate the motor slowly until that sensor shows TDC has been reached. Than becomes your counter = 0 reset point. You have to, of course, be very careful not to lose/gain pulses or you could get the situation where a piston hits a valve. This is a problem known on engines with a rubber camshaft belt which is prone to breaking.
The motor is not designed to be working in reverse. Do you have any solutions of coding for resetting the value from 2399 to 0 ?
Your code needs an overhaul, it's pretty messy and contains numerous errors!
Indentation doesn't make any difference to how the code runs, right? So why bother with it? Well, when you see code which is badly indented or not indented at all, it's always written by a beginner. Experienced coders may argue between themselves about exactly how it should be done, but they always do consistently indent their code! So you should too. Just click Tools->Auto Format. After you make changes, click Auto Format again, especially before you post it in the forum. (Thanks a lot for using code tags, by the way.)
The advice about protecting the code that reads/accesses counter is important. I can see that you tried to incorporate the necessary changes, but it's clear you have not "got it" at all. You don't need to protect access to counter in side interrupt routines, because interrupt routines cannot themselves be interrupted (at least, not on Arduino). It's code in loop(), or functions called by loop() where access to counter needs to be protected. Re-read @jremington 's post about that again.
More coding tips:
Don't use pin numbers directly in your code. What if you need to change to another pin? You will have to change the code in so many places, there's a good chance you will miss one and create a bug. Instead, define a global constant at the top of your code, and use that in place of the pin number, for example:
const byte valveA = 10;
const byte valveB = 11;
const byte encoderA = 2;
const byte encoderB = 3;
Learn about local versus global variables and variable scope. For example in this piece of code
if(digitalRead(3)==HIGH) {
counter++;
noInterrupts();
int counter_copy = counter;
}
you create a new variable called counter_copy which is a different variable to the global variable with the same name. On the next line of code, the }, that new variable is destroyed and forgotten. The global variable named counter_copy does not get changed.
But in this code
if(digitalRead(3)==LOW){
counter--;
noInterrupts();
counter_copy=counter;
}
the global variable is updated.
. . . also don't suspend interrupts in an interrupt service routine.
I've simplified the code since you dont need to handle a reverse of the motor so you need only one connection from the encoder.
volatile unsigned int counter = 0; //This variable will increase depending on the rotation of encoder
int counter_copy ;
void setup() {
Serial.begin (2000000);
pinMode(2, INPUT_PULLUP); // internal pullup input pin 2
pinMode(10, OUTPUT);
pinMode(11, OUTPUT);
//Setting up interrupt
//A falling edge pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
attachInterrupt(0, ai0, FALLING);
digitalWrite(10, HIGH);
digitalWrite(11, HIGH);
}
bool inRange(int minimum, int maximum, int val) {
return ((minimum <= val) && (val <= maximum));
}
void loop() {
noInterrupts() ;
counter_copy = counter ;
interrupts() ;
if (inRange( 0, 40, counter_copy)) {
digitalWrite(10, LOW);
}
if (inRange(580, 620, counter_copy)) {
digitalWrite(10, HIGH);
}
if (inRange(1780, 1820, counter_copy)) {
digitalWrite(11, LOW);
}
if (inRange(2365, 2395, counter_copy)) {
digitalWrite(11, HIGH);
}
if (inRange(2396, 2400, counter_copy)) {
// counter, temp = 0; // now done in ISR
}
Serial.println( counter_copy ) ; // optional
}
void ai0() {
// ai0 is activated if DigitalPin nr 2 is going HIGH to LOW
// counter reset in ISR
if ( counter == 2399 ) counter = 0 ;
else counter++ ;
}
I've done it ! It now works like a charm, the counter resets at 0 when reaching 2399:
volatile unsigned int temp, counter, counter_copy = 0; //This variable will increase or decrease depending on the rotation of encoder
void setup() {
Serial.begin(2000000);
pinMode(2, INPUT_PULLUP); // internal pullup input pin 2
pinMode(3, INPUT_PULLUP); // internalเป็น pullup input pin 3
pinMode(10, OUTPUT);
pinMode(11, OUTPUT);
//Setting up interrupt
//A falling edge pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
attachInterrupt(0, ai0, FALLING);
//B falling edge pulse from encodenren activated ai1(). AttachInterrupt 1 is DigitalPin nr 3 on moust Arduino.
attachInterrupt(1, ai1, FALLING);
digitalWrite(10, HIGH);
digitalWrite(11, HIGH);
}
bool inRange(int minimum, int maximum, int val) {
return ((minimum <= val) && (val <= maximum));
}
void loop() {
if (inRange(0, 40, counter)) {
digitalWrite(10, LOW);
}
if (inRange(580, 620, counter)) {
digitalWrite(10, HIGH);
}
if (inRange(1780, 1820, counter)) {
digitalWrite(11, LOW);
}
if (inRange(2365, 2395, counter)) {
digitalWrite(11, HIGH);
}
if (counter != temp) {
Serial.println(counter);
temp = counter;
}
if (inRange(2396, 2399, counter)) {
Serial.println("reset, 2R");
counter = 0;
temp = 0;
}
}
void ai0() {
// ai0 is activated if DigitalPin nr 2 is going from LOW to HIGH
// Check pin 3 to determine the direction
if (digitalRead(3) == HIGH) {
counter++;
}
if (digitalRead(3) == LOW) {
counter--;
}
}
void ai1() {
// ai0 is activated if DigitalPin nr 3 is going from LOW to HIGH
// Check with pin 2 to determine the direction
if (digitalRead(2) == HIGH) {
counter--;
}
if (digitalRead(2) == LOW) {
counter++;
}
}
Just two comments.
This looks a bit vague for resetting the counter. It the counter is not reset on reaching exactly 2399, pulses are going to get lost and there will be a slippage between the physical engine and your internal model of it.
if (inRange(2396, 2399, counter))
You have omitted handling possible corruption of integers which are asynchronously updated by the ISR and read in the loop(). This is most fully explained in post #5.
Thank you so much for the help ! It is now looking like it can work, I'll try mounting it on a small mono cylinder engine. Here's the "final" code:
volatile unsigned int temp, counter = 0; //This variable will increase or decrease depending on the rotation of encoder
int counter_copy;
void setup() {
Serial.begin(2000000);
pinMode(2, INPUT_PULLUP); // internal pullup input pin 2
pinMode(10, OUTPUT);
pinMode(11, OUTPUT);
//Setting up interrupt
//A falling edge pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
attachInterrupt(0, ai0, FALLING);
digitalWrite(10, HIGH);
digitalWrite(11, HIGH);
}
bool inRange(int minimum, int maximum, int val) {
return ((minimum <= val) && (val <= maximum));
}
void loop() {
noInterrupts();
counter_copy = counter;
interrupts();
if (inRange(3, 20, counter_copy)) {
digitalWrite(10, LOW);
}
if (inRange(300, 320, counter_copy)) {
digitalWrite(10, HIGH);
}
if (inRange(970, 990, counter_copy)) {
digitalWrite(11, LOW);
}
if (inRange(1250, 1270, counter_copy)) {
digitalWrite(11, HIGH);
}
Serial.println(counter_copy);
}
void ai0() {
if (counter == 1304) counter = 0;
else counter++;
}