I´m not sure if this post will end productiv …
What I want to avoid for sure is to open a public discussion battle as I often can see in forums.
But here my first impression from the software libs for the 3pi …using also Orangutan libs:
The examples are written in C … the libs came as cpp src …but finaly their coding style looks
much function oriented then OO …What I miss is any kind of class hierachie, data encapsulation
getter/setter methods … so my guess is …it has started with C …and than C++ was added later
in a way that it could be used with C++ …but not moved to an OO design.
Example could be the QTRSensor. In OO I would expect that the sensor includes all data structures and
methods it needs. By that it could easily replaced by another sensor. But I have to handover an array
sensors[5] which finally is not used outside the sensor functions. So from my perspective
I should have a sensor-object which knows how many sensors it has or …if that is the only difference
for a colection of sensors …I should just handover the number of sensors for the constructor.
What I also wonder … if I want to add an additional instance of a sensor, just assume I want to have 2 of them.
How will that work ? I guess I would have to rewrite the whole libs.
Right now I can not realy decide if I should go for a C or a C++ code.
Programming function oriented in C++ does not add much value …on the other side,
every modification to the libs needs C++ chain anyhow.
Right now, as I know C better than C++ I think about to start with a OO style in C.
Thanks for the feedback. Our AVR code is definitely very procedural and does not look very much like modern object oriented programming. However, I think we still get a lot out of C++.
For example, you asked about making multiple QTR objects. If you use C++, you can do it. In the library documentation we say,
So there you have an example of what you get from object oriented programming. In C++ it is easy for us to provide a way to set up multiple sensor arrays, while C makes this hard.
Data encapsulation is another thing that you complained about not having, but C++ does truly keep the library data encapsulated, even if you write your own code in C.
As for “getter/setter methods”, are there some specific ones that you want? A lot of them just do not make sense on robot with a tiny processor. For example, you should never need to dynamically change the number of sensors in your QTR array, since those sensors have to be physically connected to specific pins to function, and providing this functionality would use up a significant amount of the resources available on an AVR.
Finally, I don’t understand why you are complaining about having to pass in the array specifying which pins are in use by the QTR sensors. How else would you want to specify that?
Regarding the Sensor array …as mentioned … I would see it as part of the sensor object. And when I check the init function you got the numbers of pins and from that allocate the right size.
In general you can do exactly the same in C using struct and function pointers for the methods.
And …it is not about complaining. Right now it is about understanding.
Daubi, I think you and Paul are talking about two different arrays. You’re asking about the array that is passed by pointer as the first parameter to PololuQTRSensors::read and PololuQTRSensors::readLine, right?
Now I understand what you are saying, but if that array were part of the object, you would lost a lot of flexibility. For example, the memory used by the read() function would always be in use, even when you are not using the sensors. With our current method, you can make the array a local variable so that it is only used temporarily.
hm … if I do not use it than I should not call the constructor.
and the constructor might allocate the memory dynamic …if that helps.
and I would hope that gcc can remove unused libs part.
but yes this is the general discussion: design principles compared to resource usage.
last time I had to do with embeded programing like this I did it in assembler.
And event than I had 8 KB RAM, but only 1 MHz
I think you did not understand my point. If your code looks like this:
void checkLine()
{
read(...);
... do stuff with the result ...
}
int main()
{
while(1)
{
checkLine();
doOtherStuff();
}
}
then you are suggesting that the RAM need by read() should be allocated and stored in the QTR class. Say it uses 12 bytes. Then the 12 bytes will be in use when you call doOtherStuff(). With our approach, however, you would typically put those 12 bytes in a local variable defined within checkLine(), so that they would not constrain other parts of the code.
You mentioned dynamic memory allocation, but it does not really help here, it is relatively expensive compared to just using a local variable, and it can more easily lead to bugs. GCC, the way we recommend using it, does indeed remove unneeded functions, but I think that is not very relevant, since we are talking about what happens when you do use the functions.
I would keep that 12 bytes as part of the object and give back the array as pointer for the read … or keep it as part of a c struct and access it that way.
If that 12 byte are too much …than I guess you have some other problems or a lot of sensors.
might be I got something wrong …but when I have a look at the source for calibration
…the object reserves the memory …and than 12 byte is not much for the sensors …
int i;
unsigned int sensor_values[16];
unsigned int max_sensor_values[16];
unsigned int min_sensor_values[16];
// Allocate the arrays if necessary.
if(*calibratedMaximum == 0)
{
*calibratedMaximum = (unsigned int*)malloc(sizeof(unsigned int)*_numSensors);
Don´t get me wrong …I post mainly thinks where I have questions or guess some improvments.
The libs pololu provides are very useful for the start and as reference.
On the other side I see that the “style” of the libs are not always consistent regarding
data grouping, hiding, object hierarchie …
The generic approach for several Sensors and HW layouts has on one side advantages …on the
otherside it might cost performance
…e.g. the inPulse object cares for all 3 Ports …
and as the ISR needs to be as fast as posible …I would split that up in 3 instances …as the ISR
is anyhow per Port.
What I also miss in the inPulse object …add pins later …means …if my IRsensors need an ISR pins and
the Remote Control object needs an ISR pin … I would like to include the init of the ISR into the
object (sensor, RC) …right now I have to init the inpulse pins outside the objects …all together at once.
On the otherside …pins are general a central / global resource …you might better keep the pin management
outside the objects …to avoid conflicts …
The object allocates memory for calibrated values, and like anything else it temporarily uses more memory during most functions, but it does not permanently allocate the memory for storing the sensor readings made during normal operation.
Anyway, it is not clear to me whether you have any questions, but I thought of another reason not to put the readings in an ISR. With the readings being performed in the background, it becomes more difficult to calculate the I and D terms used in PID, since 0, 1, 2, or more readings might elapse between successive executions of your main loop. Of course you can probably account for that by skipping the 0’s and multiplying or dividing by the number of readings in the other cases, but that makes the logic of the program significantly more complicated.
thanks for the answer.
In general it is more a kind of code review discussion. Might be that a forum ping pong is not the best way for it.
Every single question is not that important …it makes more the sum.
And for sure there is always be an answer …as the code (your libs) works it is more a question of style and taste.
One example of the tiny questions …I did not get the point of the #include hierachie …finaly I decided to put
every src header file for the c++ code with the full path
as #include <pololu/OrangutanTime.h> do not work …compiler does not complain about the header file …but the
included classes are not found (I guess that somewhere a #ifndev is in the way)
or I wonder that the_portAMask for the QT…Sensor is private …that prevents me from writing an own ISR read function for an inherited class, protected would solve the issue.
And than the discussion might take longer, than writing my onw code …finaly my code will not be better …but suits more to my needs.
coming back to the memory allocation …as soon you enter the function calibrateOnOrOff you allocate 96 bytes for max min sensors values… …but the malloc respects the _numSensors …so works fine as long you have not more than 16 sensors …
as you already have a pointer to the max min …long time store (calibratedMaximum) …why not to use that one ? a slow pointer operation is ok for the calibration …it might hurt more during the calibrated read
also to run a cycle of 10 measurments …as you do not know what happens outside …you expect that someone cares for the black/white cases …by switching on the motors …
I mean …than 1 cycle would be ok …and if I want to have 10 cycles on the same place I had to stop the motors.
regarding the memory …for my own ISR driven sensor class …I put all data arrays as part of the class.
Than it is clear I do not have to care outside for 5x2=10 bytes …the same I do no thave to care for max min allocation outside the object. For the 3 pi it is clear that I want to have it if the object Sensors is initiated.
That I might save 10 bytes …just for the moment I do not have a measurment running …in fact they are on the stack …if I declare them in the main function …or?
Regarding the ISR I answer in the other thread, ok?
I am happy to hear your comments on the code, and I expect there are many things that could be improved about it. But I do not understand what pointer you think we should use instead of calibratedMinimum and calibratedMaximum. Those need to get allocated if they have not already been set up.
I hope that when you are finished redoing everything your way you will post your complete rewritten version to the forum! That will make your comments much more clear.
in PololuQTRSensors::calibrateOnOrOff
you store it first in the array
if(j == 0 || max_sensor_values[i] < sensor_values[i])
max_sensor_values[i] = sensor_values[i];
and later you check against the existing max value ..
if(min_sensor_values[i] > (*calibratedMaximum)[i])
(*calibratedMaximum)[i] = min_sensor_values[i];
but to be honest …when I look at the second if …I miss the point … so you are checking
if the current calibration cycle has a bigger min than the last maximum ?
what I ment … as min max are absolute numbers …you do not have to care about the history …
you can just asign it. There would be one exception …if you would take the average out of the 10
measurements …as you want to eleminate some jitter / noise. But that is not the case.
Regarding my new ISR based code … if you like I can zip it and attach it to the post?
What is missing right now is the final approval that the new lib works as line follower.
