I get that you're trying to poke fun at me
I really wasn't - I was pointing out that there are times when you need comments. I'm really not a fan of the 'comments are not needed because the code is self explanatory' school. Code only shows you what it does - not what it supposed to do, why it was written the way it is, the restrictions and limitations arising from the implementation etc. The latter are often related to scaling issues –eg. it will work for small problem sets but will fail badly if you use on medium to large sets.
Note: code example clipped slightly for brevity'
but I'd like to take this opportunity to expand on my statement.
I'm going to borrow part of the 'Dave C' example pasted above and to demonstrate how I'd split something of the sort into a separate file to make it a separate logical unit.
Here is his boxcar averaging code for measuring voltage:
for(c=0;c<39;c++) //update the moving average array
READ_VOLTAGE_ARRAY[c]=READ_VOLTAGE_ARRAY[c+1];
READ_VOLTAGE_ARRAY[39]=READ_VOLTAGE;
total=0;
for(c=0;c<40;c++)
total=total+READ_VOLTAGE_ARRAY[c];
READ_VOLTAGE_AVERAGE=total/40;
I split this filter into a separate filter.c file with its respective filter.h header.
/* filter.h */
#define FILT_BOXCAR_LEN 40
typedef struct
{
uint8_t index;
uint16_t sum;
uint16_t buffer[FILT_BOXCAR_LEN];
uint16_t avg;
} filt_boxcar_t;
extern void Filt_Boxcar_Init(filt_boxcar_t *filt);
extern uint16_t Filt_Boxcar_Sample(filt_boxcar_t *filt, uint16_t input);
/* filter.c */
/* This is where different sorts of filters are implemented */6
#include "filter.h"
void Filt_Boxcar_Init(filt_boxcar_t *filt)
{
uint8_t n;
for(n=0; n<FILT_BOXCAR_LEN; n++)
{
filt->buffer[n] = 0;
}
filt->index = 0;
filt->sum = 0;
filt->avg = 0;
}
uint16_t Filt_Boxcar_Sample(filt_boxcar_t *filt, uint16_t input)
{
filt->sum -= filt->buffer[filt->index];
filt->sum += input;
filt->buffer[filt->index] = input;
filt->index++;
if(filt->index >= FILT_BOXCAR_LEN)
{
filt->index = 0;
}
filt->avg = filt->sum / FILT_BOXCAR_LEN;
return filt->avg;
}
So to use this in Dave's project:
filt_boxcar_t filter_volt;
filt_boxcar_t filter_curr;
Filt_Boxcar_Init(&filter_volt);
Filt_Boxcar_Init(&filter_curr);
/* places in the code where the ADC samples are captured and need filtering*/
/* see the original code we attempted to replace */
READ_VOLTAGE_AVERAGE = Filt_Boxcar_Sample(&filter_volt, READ_VOLTAGE);
/* a bit lower */
READ_CURRENT_AVERAGE = Filt_Boxcar_Sample(&filter_curr, READ_CURRENT);
/* note: I'm not a fan of using all caps for variables, as those are usually reserved for macros, but it is what it is */
Please don't take this personally as I understand you were just providing a quick, rather than a fully engineered example, but I hated your solution! But note that: this is as it applies to this specific case - robust, optimized library solutions for common problems can avoid developers continually reinventing the wheel with their own unique, new buggy versions. They are often a very good idea.
But your example doesn't, being inflexible, fragile and worse, produces the wrong results. It doesn't help me as someone in the position of writing Dave's original code - it makes my life harder!
[EDIT] In the following scenario consider the case where I am working in a large company on a somewhat larger project than the usupply, using the same type of filter.
First up: Dave's original solution is simple, probably took a few minutes to write, does exactly what he needed, and importantly, because he wrote it he almost certainly understood its behaviour and limitations and would have had a good idea about the speed and memory footprint. It may not be the fastest but who cares if the processor only spends 95% of its time idling instead of 95.5%?
OTOH, when management decrees that you (ie. the other Dave, who proposed the above code) will be responsible for providing filter solutions for all development teams and those must be used unless there is a strong justification for not doing so, I lose control of that part of my code.
Seeing that I have already implemented a pair of moving average filters, you (the filter team) base your official version on that - because it clearly meets my requirements, and I'm told, in line with best practices in the company's software development processes that I have to use those instead of my likely buggy version as I'm not the resident 'filter expert'.
With luck you will have provided some documentation and/or example code to show how it's used along with a custom header file configuration tool - to prevent any none-greybeards editing 'buffer length' macro definitions in the header files because the coding/development standards demand that only level 7 and above consulting language experts can do so as it is well known that macros are responsible for 43.7% of all bugs.
So now I have to spend time locating all the above as well as the header files and code, ensuring they are all at the right revision level by using the code and document repository management tools to request access. With luck, within a couple of days it will be authorised and my access rights updated to read-only. Helpfully, henceforth, I will be sent regular emails to inform me about the latest spelling correction revisions to the user documentation.
To use your solution, although I'm told it's thoroughly tested and ‘Purified’ (is that still a thing these days?), that was on the filter team's 64 bit machines - it hasn't been built or tested for the 8-bit micro target environment I am using. Thus I'm forced to read through your code to ensure that it does what I need with no pitfalls or unexpected side-effects. But your code, with the extra indirections necessary to make it reusable is a bit harder to read and takes me almost as long as writing the original code.
At this point I'm not happy. The official moving average filter, unlike mine, uses an extra 5 bytes, possibly 6 with padding, of my precious, *very* limited static RAM resource for each instantiation. I had planned to add several new few short filters to my code, but the extra overhead has killed that plan so I'll have to see if we can work-around the noise problems some other way.
This improvement was obviously done as a speed optimization which would be ‘much, much better-er’. Except that I don't have a speed problem I have a memory problem! Worse, the filter team decided it would be a good idea to include an additional structure element ‘avg’ to keep a copy of the last computed average value just in case you forgot to save your own copy and should you happen to need it again. I don't need it but in the many requirements review meetings I was requested to attend, other teams - who almost certainly will never require any sort of filter - agreed "it might be useful". They went on to demand various kitchen sink parts also be included but fortunately there was no consensus on whether it should be taps, drainer or waste fittings so luckily we managed to avoid the worst.
On a second look you realise the filter doesn’t actually work as it was clearly only ever tested with small input values. My version (as did Dave’s of EEVBlog fame) used a long type for the sum variable whereas the improved one only uses a uint16_t. Perhaps they were in touch with their 'embedded side' after all, trying to save memory and CPU cycles. Of course, although my voltage and current samples come from a 10 bit ADC and the sum of 40 of them will actually fit into a uint16_t, my code rescales the input values which means both the Voltage and Current filters can overflow. That's not good but even worse is that my original version, had it also used an uint16_t for the sum, it would at least have recovered after 40 more samples provided the input values weren’t too large thus providing some sort of work-around to the customer. The new improved version is so fragile that once the sum value becomes corrupted it will never recover.
So now I've got to either change my code or fight my way through the change control process to get the filter fixed, though their opening stance is that the code was self explanatory - it was quite clear that input values > 65536/40 were not acceptable, and there were no comments to the contrary. You should have scaled your input accordingly. Eventually they concede that it could be improved (but they were never wrong) and agree to release an update in due course.
[WARNING - even more contrived section - skip if you have a life]You get the update which now accounts for the fact that the maximum value of the sum variable depends on the length of the filter as well as the amplitude of the input samples. Because there is no maximum filter length limitation in the API documentation (that would have required a series of meetings with the users to agree a change to the API) the filter team team decided to implement dynamic sizing for ultimate flexibility. Now the sum is initially a 16 bit value (for efficiency on 8 or 16 bit targets) but when it gets close to the limit it switches to a 32 bit type. Best of both! What's not to like - efficient and adaptable?
As a bonus the APi now allows you to specify the overload behaviour - either take the hit with the 32-bit accumulator or switch to saturating logic to retain the speed and code size advantage of a 16-bit accumulator. Naturally you have to pay with a few more dozen of your precious static RAM bytes but clearly it's worth it, Look how shiny those saturation values are...
[/WARNING - even more contrived section - skip if you have a life]So we are all done... please? Ermm no. you don't get away that easily; you see I get to the point where, as I suspected, the current channel turns out to be noisier than hoped so it requires a longer filter. The buffer length is currently defined using a macro in the header file; problem is that in the interest of code safety, as per MISRA rules, dynamic memory allocation isn’t allowed. The user will now have to explicitly provide statically allocated filter buffers. The necessary API changes mean new rounds of change requests but you win the case eventually because they can see the whites of your eyes and your bared canines.
Now the filter construction in my code requires:
#define VOLTAGE_ FILT_BOXCAR_LEN 40
#define CURRENT_ FILT_BOXCAR_LEN 70
filt_boxcar_t filter_volt;
filt_boxcar_t filter_curr;
uint16_t voltage_buffer[VOLTAGE_FILT_BOXCAR_LEN];
uint16_t current_buffer[CURRENT_FILT_BOXCAR_LEN];
Filt_Boxcar_Init(&filter_volt, &voltage_buffer, VOLTAGE_ FILT_BOXCAR_LEN);
Filt_Boxcar_Init(&filter_curr, ¤t_buffer, CURRENT_ FILT_BOXCAR_LEN);
Unfortunately the filter structures now have to include the filter length and pointers to the actual buffer. It will all crash and burn spectacularly if I get it slightly wrong but that's a small price to pay for the dramatic improvements from the provision of rugged and reliable standard library implementations. What an idiot I was thinking I could write my own filter in 10 minutes.
For those who haven't given up the will to live by this point I'll spare you the issues arising from the design change requiring the need to update the Current filter within an interrupt routine. Suffice it to say that it isn't pretty. And the solution involved writing my own obscurated filter in assembler code labelled 'Definately_not_a_moving_average_filter', cunningly disguised as a delay loop to match hardware timings. It should work as almost nobody else knows this processor’s assembler code. I have to hope that smart-arse, up and coming, looking to make a name for himself Rodgers isn’t on the code review team....
And worst of all. because I clearly have some limited skills in coding filter I have now been enrolled into the filter team design review meetings....
As you can see, the same filter function is called for filtering both values. Not only that, the same filter module can have multiple filters implemented inside and all of them can then be elegantly used wherever they're needed in different sections of code. Making things modular may not be strictly necessary in small pieces of code, like just testing some proof of concept with a couple hundred lines, but it is absolutely crucial in larger projects.
Thanks. You turned 10 minutes work into 8 weeks of fruitless arguing with management and the filter team that their ’elegant solution’ was a POS and totally unsuitable for my needs. The extra static RAM had to come from the stack allocation so fingers crossed that an I2C interrupt never occurs during a remote configuration mode change in debug mode.
I don't need different filters, I don't need code re-use for other projects, I just needed 7 lines of code to get a simple job done.
Right, exactly what I said at the end of my post.
Making things modular may not be strictly necessary in small pieces of code, like just testing some proof of concept with a couple hundred lines, but it is absolutely crucial in larger projects.
But the ‘elegant solution’ is just as rubbish and unnecessary in a 500K LOC project as in a simple test program. Yes we know you were illustrating the concept of reusability with a simple example but it is definitely not always ‘absolutely crucial’ no matter how big the project. Even complex functions that look like they should be provided in a library shouldn't always - especially in embedded, details matter, often performance related. There are always costs as well as advantages.
Please don't claim that I'm advocating that every team writes all their own code and reuse is bad. Its horses for courses. Now if your talking polyphasing decimator FIR implementations I probably don’t want to write my own. In C or assembler...
Home
Help
Search
Login
Register
