Electronics > Projects, Designs, and Technical Stuff

uSupply Firmware Released

<< < (7/11) > >>

Dave:

--- Quote from: splin on May 27, 2020, 01:41:12 am ---
--- Quote from: Dave on May 27, 2020, 12:53:04 am ---Good code usually doesn't even need comments, because the names of the functions and variables themselves are descriptive enough to make it clear what something is doing.

--- End quote ---
int Check_DMA_NotAboutToAcessSPI_CMD_RegisterWhilstUART1_TX_CompleteFlagSet_ToAvoidLockup_DescribedInErrata3.16.7(void);

--- End quote ---
I get that you're trying to poke fun at me, 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:

--- Code: ---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;

--- End code ---

I split this filter into a separate filter.c file with its respective filter.h header.

--- Code: ---/* filter.h */
/* The header file of the filter module, which should be #included at the top of main.c */

#ifndef FILTER_H
#define FILTER_H

#include <stdint.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);

#endif /* FILTER_H */
--- End code ---


--- Code: ---/* filter.c */
/* This is where different sorts of filters are implemented */

#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;
}
--- End code ---

So to use this in Dave's project:

--- Code: ---/* top of file */
#include "filter.h"

/* variables definition */
filt_boxcar_t filter_volt;
filt_boxcar_t filter_curr;

/* initialization routine */
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 */
--- End code ---

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.

jbb:
Oh no, now I'm doing it...

... and to extend that to hypothetical C++, it gets quite tidy.


--- Code: ---/* top of file */
#include "filter.hpp"

/* filter objects definition (self-initialising) */
filt_boxcar_t filter_volt;
filt_boxcar_t 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 = filter_volt.update(READ_VOLTAGE);
/* a bit lower */
READ_CURRENT_AVERAGE = filter_curr.update(READ_CURRENT);

--- End code ---

In the end, this will compile to much the same thing as the C option*.  The filter update semantics quite nicely tie the filter data storage to the filter update function.  The initialisation is done implicitly by the constructor which is often pleasant but sometimes a pain to control precisely.  I've had good results with this sort of thing for signal processing stuff like filters, PI controllers etc.

However, if we use advanced features like exceptions and class polymorphism the resource usage (RAM, ROM, time) can suddenly go up.  Maybe you really need those features and are willing to pay for them. Or maybe you don't, and are wondering why the binary size has exploded.

I've never done much with templates, but I understand that some compilers (once upon a time, a someone told my mate's cousin at the pub) might not quite understand which templates you were using, and simply generate the template code for all possible types.  Which makes your binary explode.  Hopefully newer compilers aren't that stupid.

* If we were being anal retentive, we could call to the fact that the C Filt_Boxcar_Sample() isn't checking for null pointers, and adding run time checks would consume resources.  The compiler handles the pointer for the C++ version so you don't need a run time check.

EEVblog:

--- Quote from: SiliconWizard on May 27, 2020, 02:01:41 pm ---That's one more reason the choice of using C++ in this project, at least the way Dave used it, was probably not a good idea. Days of delay for tools issues - that's OK. But weeks? You were probably not in a big hurry at the time, and wanted to give him maximum autonomy, but as a manager, you should probably have made a couple decisions to limit useless delays.
--- End quote ---

Such a decision is really only possible in hindsight.
Each small problem is always presented/appears as being the "last hurdle". Then you have another one, and another one, and another one, and before you know it you are too far down the process to change direction. Such a change in direction would have meant rewriting most of it in the "DaveC" way, a method that David is not familiar with and would actually hate to do. It's not good to piss off your engineering employees and make them do something they don't want to do, and effectively saying "you way is shit, do it this way because I say so", it's better to give them autonomy to get the job done using methods they are familiar with and think will do the best job.

Also, because I'm no expert in C++ and the tools David used to do all this, I had to essentially trust him and his judgement, and the end result is entirely what David wanted and the way he produces code.

With hindsight now, yes I would have insisted that he use the plain vanilla ST tools and regular C, and set better code targets. Just look at the video we did just setting up the tools and environment he used, it's very complicated and one that was ultimately not needed for such a relatively small embedded project. But it's what he was familiar with being an uber code nerd.

EEVblog:

--- Quote from: Dave on May 27, 2020, 11:14:11 pm ---
--- Quote from: splin on May 27, 2020, 01:41:12 am ---
--- Quote from: Dave on May 27, 2020, 12:53:04 am ---Good code usually doesn't even need comments, because the names of the functions and variables themselves are descriptive enough to make it clear what something is doing.

--- End quote ---
int Check_DMA_NotAboutToAcessSPI_CMD_RegisterWhilstUART1_TX_CompleteFlagSet_ToAvoidLockup_DescribedInErrata3.16.7(void);

--- End quote ---
I get that you're trying to poke fun at me, 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:

--- Code: ---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;

--- End code ---

I split this filter into a separate filter.c file with its respective filter.h header.

--- Code: ---/* filter.h */
/* The header file of the filter module, which should be #included at the top of main.c */

#ifndef FILTER_H
#define FILTER_H

#include <stdint.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);

#endif /* FILTER_H */
--- End code ---


--- Code: ---/* filter.c */
/* This is where different sorts of filters are implemented */

#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;
}
--- End code ---

So to use this in Dave's project:

--- Code: ---/* top of file */
#include "filter.h"

/* variables definition */
filt_boxcar_t filter_volt;
filt_boxcar_t filter_curr;

/* initialization routine */
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 */
--- End code ---

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.

--- End quote ---

Yeah I do do that, but it's neither here nor there with a project like this. 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.

EEVblog:

--- Quote from: SiliconWizard on May 27, 2020, 02:01:41 pm ---Now to be clear, OTOH I find Dave (Jones) coding style pragmatic, but a bit too simplistic and not the most reusable or most easy to maintain. My take would be somewhere in between.

--- End quote ---

For a bigger project I would have used multiple files and split things up more.
In fact I sometimes do this as a 2nd pass once a project gets bigger. I'm not a fan of putting in a lot of modular coding work for a project what I knew would only need a few hundred lines of code to get a working prototype, and maybe a few hundred more to refine a final product.

Navigation

[0] Message Index

[#] Next page

[*] Previous page

There was an error while thanking
Thanking...
Go to full version
Powered by SMFPacks Advanced Attachments Uploader Mod