I need to implement a custom feedback controller and I was wondering whether there exist any good design patterns. With the following code I can achieve very high sampling rates, but I have trouble outputting the controller's action fast enough. Actually, what I currently get is a mere 500Hz feedback rate. Here's my code:
#include <Arduino.h>
#include <avr/io.h>
#include <avr/interrupt.h>
volatile uint16_t myMeasurement = 0;
volatile uint16_t *myPointer = &myMeasurement;
void setup(void)
{
DDRC &= ~(1<<DDC0); // Pin A0 as Input
DDRD |= (1<<PIND7);//Pin 6 on arduino as output
ADCSRA |= 1<<ADPS2 | 1<<ADPS1;// Prescaler=64
ADMUX |= (1<<REFS0) | (1<<REFS1); // Internal 1.1V reference
ADCSRB &= ~((1<<ADTS2)|(1<<ADTS1)|(1<<ADTS0));//ADC in free-running mode
ADCSRA |= (1<<ADATE);//Signal source, in this case is the free-running
ADCSRA |= 1<<ADIE; // Enable the interrupt
ADCSRA |= 1<<ADEN;// Enable the ADR
sei();// Enable Interrupts (Global)
ADCSRA |= 1<<ADSC;//Start first conversion (necessary always)
}
void loop(){
while(1){
uint16_t currentState = *myPointer;
uint16_t controllerAction = calculateControllerAction(currentState);
PIND ^= 1<<PIND6;//flip pin 6 on arduino
delay(1);
}
}
// Method to calculate the controller's action
uint16_t calculateControllerAction(uint16_t currentState){
uint8_t gain = 2;
return currentState * gain;
}
// ADC Interrupt method
ISR(ADC_vect)
{
uint8_t lowPart = ADCL;
*myPointer = ADCH<<8 | lowPart;
}
With the delay(1) at the end of our loop() method we get a proper square wave @496Hz as expected. Can we do better that this? Is there a way to avoid using delays or even the method millis()? Are there some general best-practices for the implementation of custom controllers using arduino?
Note: Consider an implementation like this:
long interval = 1;
long previousMillis = 0;
void loop(){
while(1) {
unsigned long currentMillis = millis();
uint16_t currentState = *myPointer;
uint16_t controllerAction = calculateControllerAction(currentState);
controllerAction = calculateControllerAction(currentState); // lengthy operation (e.g. online optimisation)
if (millis()-currentMillis>interval){//Time Exceeded already!!!
interval = millis()-currentMillis;//update/increase the interval...
}
while(currentMillis - previousMillis < interval) {//wait for the time to come...
currentMillis = millis();
}
previousMillis = currentMillis;
//analogWrite(6,map(controllerAction,0,2046,0,255));
PIND ^= 1<<PIND6;//flip pin 6 on arduino
}
}
Is this acceptable if we need to strike a constant rate of actuation? Again, what I want to know is whether there is some standard way to go.
void setup(void)
{
DDRC &= ~(1<<DDC0); // Pin A0 as Input
DDRD |= (1<<PIND7);//Pin 6 on arduino as output
ADCSRA |= 1<<ADPS2 | 1<<ADPS1;// Prescaler=64
/* The internal reference takes a several microseconds to "warm up". The remaining few lines of code take maybe 2 microseconds to execute. The first analog reading is very likely garbage. I suggest putting a delay after enabling the internal reference. */
ADMUX |= (1<<REFS0) | (1<<REFS1); // Internal 1.1V reference
ADCSRB &= ~((1<<ADTS2)|(1<<ADTS1)|(1<<ADTS0));//ADC in free-running mode
ADCSRA |= (1<<ADATE);//Signal source, in this case is the free-running
ADCSRA |= 1<<ADIE; // Enable the interrupt
/* From the datasheet: Voltage reference and input channel selections will not go into effect until ADEN is set. Setting ADEN should be the first thing you do. */
ADCSRA |= 1<<ADEN;// Enable the ADR
/* The following line of code is pointless. Interrupts are already enabled */
sei();// Enable Interrupts (Global)
ADCSRA |= 1<<ADSC;//Start first conversion (necessary always)
}
Is this acceptable if we need to strike a constant rate of actuation?
In general, uneven sampling rates translate into different gains. So you want it to be the same. In general, that's assured by constant loop execution time, or via a timer.
In general, a feedback loop's stability is very difficult to be designed to be stable under all conditions. Usually reaction speed and stability are mutually exclusive.
With 1ms you end up with 1/2kHz, not 1kHz because every time you flip the pin once, not twice.
I use it when I comment out the line PIND ^= 1<<PIND6; to replace it with analogWrite(6,map(controllerAction,0,2046,0,255));. But, PIND ^= 1<<PIND6; shows me the frequency of actuation.
No particular meaning :). I was just playing around!
Should I put the delay after ADMUX |= (1<<REFS0) | (1<<REFS1); or after ADCSRA |= 1<<ADEN;// Enable the ADR ? I think the delay should be placed after the ADC is enabled; isn't it?
I don't see any problem with that. First I specify the voltage reference and then, I start the ADC with the desired configuration. If ADEN is not set, the voltage reference and the input channel don't make any sense - the ADC is shut down.
PINx is the input data register - it is read only. Writing 1 to it activates the pull-up, and writing 0 to it has no effect. What happened here is that PORTx (the output latch) is clear by default ('0'). So when you write 1 to PINx, the line is pulled up by the internal pull-up resistor (very weak, and suspect to parasitics on the line).
Always write to PORTx for digital output. Only use PINx if you actually want to read the pins.
dhenry:
PINx is the input data register - it is read only. Writing 1 to it activates the pull-up, and writing 0 to it has no effect. What happened here is that PORTx (the output latch) is clear by default ('0'). So when you write 1 to PINx, the line is pulled up by the internal pull-up resistor (very weak, and suspect to parasitics on the line).
Always write to PORTx for digital output. Only use PINx if you actually want to read the pins.
Read the datasheet for that.
As to ADC, read the device header file.
As to template, no.
dhenry:
Once again you are mistaken, and created a henry always rule that is at odds with what the chip can actually do. From the 328p datasheet:
14.2.2 Toggling the Pin
Writing a logic one to PINxn toggles the value of PORTxn, independent on the value of DDRxn.
Note that the SBI instruction can be used to toggle one single bit in a port.
So writing a one to a PINx bit does toggle the PORTx pin and can be a most efficient instruction and that is a really true data output, not a weak pull-up, if the mode is as a output pin. Note that older AVR chips did not have this additional 'toggle' output bit behaviour.
dhenry is right. The PORT is output, the PIN is input. However that might work on the grounds that you are toggling the last thing you set the port to. So that could be OK.
The pointer doesn't achieve anything except waste time.
You might also want to put accessing the variable into a critical section, eg.
void loop(){
while(1){
// protect access to 16-bit variable
noInterrupts ();
uint16_t currentState = myMeasurement;
interrupts ();
// OK we can proceed
uint16_t controllerAction = calculateControllerAction(currentState);
PIND ^= 1<<PIND6;//flip pin 6 on arduino
delay(1);
}
}
The ADC reads take 104 uS normally (unless you changed the reading rate) so you should be able to do 1/0.000104 per second, namely 9615 reads per second.
With 1ms you end up with 1/2kHz, not 1kHz because every time you flip the pin once, not twice.
Ah I fell for that old trick. Well you are getting around 1000 reads per second because of the 1 mS delay. So?
I checked using the pointer and accessing the variable directly. There is no (obvious) effect on the performance. Besides, isn't it more resource-efficient to use pointers in the sense that the variable myMeasurement has a predefined position in the memory and no new bits will be reserved for it.
Why is that? It is for sure that myMeasurement will change, many a time, before the line PORTD ^= 1<<PIND6;. We just capture a value from myMeasurement. I don't really understand the need for making this variable access-safe here.
Is this the best I can get out of my Arduino? It looks satisfactory for most control applications. But, can I do even better?
chung:
Besides, isn't it more resource-efficient to use pointers in the sense that the variable myMeasurement has a predefined position in the memory and no new bits will be reserved for it.
No, you are forcing a pointer dereference when you don't need one.
chung:
[quote author=Nick Gammon link=topic=131961.msg992616#msg992616 date=1352683994]
You might also want to put accessing the variable into a critical section, eg.
Why is that? It is for sure that myMeasurement will change, many a time, before the line PORTD ^= 1<<PIND6;. We just capture a value from myMeasurement. I don't really understand the need for making this variable access-safe here.
[/quote]
Because the processor writes one byte and then the other one (in the unsigned int). You may catch it between writes. Say it was 0x1234 and is now 0x4567. If you catch it half-way through you might get 0x1267 or 0x4534. Neither of which is correct.
chung:
[quote author=Nick Gammon link=topic=131961.msg992616#msg992616 date=1352683994]
Well you are getting around 1000 reads per second because of the 1 mS delay. So?
Is this the best I can get out of my Arduino? It looks satisfactory for most control applications. But, can I do even better?
[/quote]
What do you mean? If you put the 1 mS delay, yes. What is the purpose of the delay? Make it 100 uS and see what happens. As I said it takes 104 uS to do the ADC conversion, so around 100 uS is about your limit. Plus you can get dedicated ADC chips that can output results about every 4 uS. They aren't very expensive.
You can speed up the ADC conversion (I can't tell right now if you have done that) by dropping the resolution. This may or may not matter to you.
Except for speed I want to be able to control the actuation frequency and keep it (close to) constant. This is why I applied this delay. Without the delay I can have pin 6 changing state at the frequency of 723~730kHz; which is pretty high but a bit fluctuating all the time. The delay, and the way I implemented it serves the purpose of stabilizing the actuation frequency and nothing but that. Is it possible to tell Arduino to wait for less than 1ms?
Also, implementing a timer interrupt for applying or even calculating the control action isn't wise because both (and especially the calculation of the control action) can be rather time consuming.
Some (potentially constructive) brainstorming: At some point, I wrote:
if (millis()-currentMillis>interval){//Time Exceeded already!!!
interval = millis()-currentMillis;//update the interval...
}
This has a fundamental drawback: if, for any reason, the invocation calculateControllerAction(currentState); turns out to be too slow then the actuation interval will become immediately very high. Changing this with the average execution time for calculateControllerAction or the 3rd percentile of the distribution of execution times would give better results.
chung:
Should I put the delay after ADMUX |= (1<<REFS0) | (1<<REFS1); or after ADCSRA |= 1<<ADEN;// Enable the ADR ? I think the delay should be placed after the ADC is enabled; isn't it?
Yes. To ensure the code is now and will always be correct you should delay after the internal voltage reference is turned on; which is after setting ADMUX / REFSn and setting ADCSRA / ADEN.
[quote author=Coding Badly link=topic=131961.msg992554#msg992554 date=1352678689]/* From the datasheet: Voltage reference and input channel selections will not go into effect until ADEN is set. Setting ADEN should be the first thing you do. */
ADCSRA |= 1<<ADEN;// Enable the ADR
I don't see any problem with that. First I specify the voltage reference and then, I start the ADC with the desired configuration. If ADEN is not set, the voltage reference and the input channel don't make any sense - the ADC is shut down.[/quote]
I was under the impression that changing ADC registers was restricted until ADEN was set. I was wrong. What you are doing is not a problem.
However, moving the ADCSRA / ADEN and ADMUX / REFSn code to the earliest possible point allows the internal voltage reference to warm-up while you perform other initialization; the warm-up can be done in parallel with other things. If that matters in your application.