Author Topic: uSupply Firmware Released  (Read 13714 times)

0 Members and 4 Guests are viewing this topic.

Offline Jeroen3

  • Super Contributor
  • ***
  • Posts: 4209
  • Country: nl
  • Embedded Engineer
    • jeroen3.nl
Re: uSupply Firmware Released
« Reply #25 on: May 27, 2020, 10:06:33 am »
Even in the tiny forum window I can figure out what's going on.. This must be hardware vs. software engineering thinking ways.
Script hackers vs SOLID developers.
https://nl.wikipedia.org/wiki/SOLID
 

Online SiliconWizard

  • Super Contributor
  • ***
  • Posts: 15800
  • Country: fr
Re: uSupply Firmware Released
« Reply #26 on: May 27, 2020, 02:01:41 pm »
Anonymous function (refered as authored by klingon) probably overkill for embedded and exactly opposite "of easy to understand code".
And whole fact returning pointer to *char by anonymous function might be fine on PC, but ehm, i think questionable in embedded.
But thats IMHO, i might be wrong.
I generally try to be careful and conservative with C++ features in embedded, you never know which function will make your code "suddenly overweight" or will start using "under the hood" dynamic memory allocation/reentrant functions/etc and guarantee you problems.
But it’s best to prove that, if i can write better code. Sure if i will be alive by the time uSupply will be released for sales.

The other thing is that David kept finding issues in the C++ compiler (and STM target), as in bugs or quirks that prevented the program from working, and in one case resulted in weeks worth of project delay until the compiler could be patched.

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.

Again, I admire the clarity and elegance of his code, but I still question the choices and architecture. A bit too bloated for my taste.

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.
 

Offline nuclearcat

  • Supporter
  • ****
  • Posts: 382
  • Country: lb
Re: uSupply Firmware Released
« Reply #27 on: May 27, 2020, 06:10:54 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.
For me it is pretty typical AVR Studio code, "It just works(tm)", similar to code in Atmel datasheets.
(And half of code is unavoidable registers voodoo).
 

Offline MiroS

  • Regular Contributor
  • *
  • Posts: 175
  • Country: pl
Re: uSupply Firmware Released
« Reply #28 on: May 27, 2020, 06:55:29 pm »
As soon as software becomes slightly more complex, you need to start splitting it in logical units, which you put into separate files. A well written software should not have files that stretch for more than a couple of hundred lines. There are of course always exceptions to this rule, but in general, one shouldn't have to scroll around and search for a particular part of code.

If you think  than please take this as home assigement    https://ptolemy.berkeley.edu/projects/embedded/pubs/downloads/spice/spice2.ibm.tar.gz and come back where  is more clarity and complexity :popcorn:

 

Offline Dave

  • Super Contributor
  • ***
  • Posts: 1356
  • Country: si
  • I like to measure things.
Re: uSupply Firmware Released
« Reply #29 on: May 27, 2020, 10:14:58 pm »
Anonymous function (refered as authored by klingon) probably overkill for embedded and exactly opposite "of easy to understand code".
And whole fact returning pointer to *char by anonymous function might be fine on PC, but ehm, i think questionable in embedded.
But thats IMHO, i might be wrong.
I generally try to be careful and conservative with C++ features in embedded, you never know which function will make your code "suddenly overweight" or will start using "under the hood" dynamic memory allocation/reentrant functions/etc and guarantee you problems.
There are some aspects of C++ that I think would be useful for embedded platforms, like operator overloading, constructors and destructors, default arguments in functions. Other than that...  :-//
I'm not terribly fond of anonymous functions, either.

If you think  than please take this as home assigement    https://ptolemy.berkeley.edu/projects/embedded/pubs/downloads/spice/spice2.ibm.tar.gz and come back where  is more clarity and complexity :popcorn:
You linked in an ancient project coded in FORTRAN (I only know this because I actually searched for the SPICE wiki). I can't possibly comment on a programming language which I don't know and have never used. If the language does allow for splitting the code into multiple files, then 18k lines in a single file is in fact terrible coding practice.
<fellbuendel> it's arduino, you're not supposed to know anything about what you're doing
<fellbuendel> if you knew, you wouldn't be using it
 

Offline Dave

  • Super Contributor
  • ***
  • Posts: 1356
  • Country: si
  • I like to measure things.
Re: uSupply Firmware Released
« Reply #30 on: May 27, 2020, 11:14:11 pm »
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.
int Check_DMA_NotAboutToAcessSPI_CMD_RegisterWhilstUART1_TX_CompleteFlagSet_ToAvoidLockup_DescribedInErrata3.16.7(void);
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: [Select]
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.
Code: [Select]
/* 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 */

Code: [Select]
/* 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;
}

So to use this in Dave's project:
Code: [Select]
/* 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 */

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.
<fellbuendel> it's arduino, you're not supposed to know anything about what you're doing
<fellbuendel> if you knew, you wouldn't be using it
 

Offline jbb

  • Super Contributor
  • ***
  • Posts: 1265
  • Country: nz
Re: uSupply Firmware Released
« Reply #31 on: May 28, 2020, 12:34:51 am »
Oh no, now I'm doing it...

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

Code: [Select]
/* 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);

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.
 

Online EEVblogTopic starter

  • Administrator
  • *****
  • Posts: 39026
  • Country: au
    • EEVblog
Re: uSupply Firmware Released
« Reply #32 on: May 28, 2020, 05:43:13 am »
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.

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.
« Last Edit: May 28, 2020, 05:55:34 am by EEVblog »
 
The following users thanked this post: hans, croma641

Online EEVblogTopic starter

  • Administrator
  • *****
  • Posts: 39026
  • Country: au
    • EEVblog
Re: uSupply Firmware Released
« Reply #33 on: May 28, 2020, 05:47:53 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.
int Check_DMA_NotAboutToAcessSPI_CMD_RegisterWhilstUART1_TX_CompleteFlagSet_ToAvoidLockup_DescribedInErrata3.16.7(void);
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: [Select]
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.
Code: [Select]
/* 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 */

Code: [Select]
/* 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;
}

So to use this in Dave's project:
Code: [Select]
/* 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 */

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.

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.
 

Online EEVblogTopic starter

  • Administrator
  • *****
  • Posts: 39026
  • Country: au
    • EEVblog
Re: uSupply Firmware Released
« Reply #34 on: May 28, 2020, 05:54:32 am »
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.

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.
 

Offline Dave

  • Super Contributor
  • ***
  • Posts: 1356
  • Country: si
  • I like to measure things.
Re: uSupply Firmware Released
« Reply #35 on: May 28, 2020, 07:19:00 am »
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.
<fellbuendel> it's arduino, you're not supposed to know anything about what you're doing
<fellbuendel> if you knew, you wouldn't be using it
 

Offline Jeroen3

  • Super Contributor
  • ***
  • Posts: 4209
  • Country: nl
  • Embedded Engineer
    • jeroen3.nl
Re: uSupply Firmware Released
« Reply #36 on: May 28, 2020, 10:30:28 am »
Well, the thing is that developers write code to handle change of requirements. So from programmers point of view I think there are three major objectives:
1. The code must pass the tests (aka the requirements).
2. The code must allow change of requirements at low cost.
3. The code must be maintainable, other people need to be able to read it.

To try and make this easy we have invented Object Oriented Programming and design patterns to use to conform to the solid rules.

What we call "DaveC" here only passes requirement 1. And requirement 3 for the first few weeks. After 2 iterations of rule 2 and it's too big. Read up on "why software fails".

Now the problem here is that many embedded software developers are also electrical engineers used to making hardware. Changes to hardware are always expensive. So changes are less common.
Also the embedded software is often named firmware. Firm isn't hard, but also not soft. It's in between.

It's perfectly fine to write a small bit of code that makes a chip do a simple job and to never change it again. Your quarts watch for example. There DaveC is perfectly acceptable if you can test it.

It's another thing to write a reasonably chunk of code doing a simple job fow now with the idea to later improve upon it. Your Apple Watch.

This is in my opintion the biggest weak spot of the embedded world. Many people who write code for embedded are not career developers. And many career developers do not know how to write code that works on embedded. With the above shown "figh" about the "best code".

However, compared to the target audience, I can see the concern here. The uSupply with this architecture isn't a project average joe who bought an arduino can get on with. But does it need to be?

(3 is unfortunately largely forgotten by many people)


I'm currently developing in Qt C++. And it's hard, as embedded engineer. With Qt you are forced to comply to SOLID and design patterns. Trying "DaveC" fails after about 5 features.
« Last Edit: May 28, 2020, 10:34:53 am by Jeroen3 »
 
The following users thanked this post: hans

Online SiliconWizard

  • Super Contributor
  • ***
  • Posts: 15800
  • Country: fr
Re: uSupply Firmware Released
« Reply #37 on: May 28, 2020, 02:24:07 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.

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.

Believe me, I know exactly what you're saying here! That actually defines a technical manager's job pretty well. We're confronted with that all the time. "Last hurdle"? Yes, I've heard that too so many times!  ;D

And with experience as a manager, you learn how to spot that and anticipate. Correctly monitoring the project will help not waiting until it's too late to change directions.

It may not have meant rewriting everything from scratch either! I'm pretty sure using a simpler subset of C++ (which gets back to other discussions on C++ for embedded targets) would have been enough to get out of the rut without Dave 2 having to change all his code base or switch to pure C. Not using the problematic C++ constructs would have been enough, and probably not *that* difficult to do. He may have "wasted" one week instead of several without moving forward. Now his insisting on still using them while it made him waste weeks, that wasn't good. Hopefully, he's learned a few things along the way!

That's IME one of the hardest parts of tech management. Sometimes you need to make the decisions for others, even when your engineers are swearing they know what they're doing and basically ask you to leave them alone. Interestingly, if you don't make the required decisions (even when they are hard to make), your engineers (at least some of them) will end up resenting you! As a manager, you're expected to make decisions and give them guidance - which is hard to do, because right when you do that, they will usually try their best to argue you're being wrong. Just sharing my 2 cents of experience here. ^-^

« Last Edit: May 28, 2020, 02:25:48 pm by SiliconWizard »
 
The following users thanked this post: Vovk_Z

Offline VEGETA

  • Super Contributor
  • ***
  • Posts: 2018
  • Country: jo
  • I am the cult of personality
    • Thundertronics
Re: uSupply Firmware Released
« Reply #38 on: May 28, 2020, 10:10:43 pm »
what about the hardware circuit?  < million dollar question for +10 years xD

Offline uer166

  • Super Contributor
  • ***
  • Posts: 1026
  • Country: us
Re: uSupply Firmware Released
« Reply #39 on: May 29, 2020, 02:00:03 am »
what about the hardware circuit?  < million dollar question for +10 years xD

Don't care for the USB whatever, but I really would like to see the video about the design of that planar transformer and the actual converter itself in detail.
 

Offline Gonzy78

  • Contributor
  • Posts: 10
  • Country: fr
Re: uSupply Firmware Released
« Reply #40 on: May 29, 2020, 10:10:20 am »
Dear Dave,

Will it be open hardware ?
I am quite interested you detail the input USB-C part of the schematic. This is neat stuff. USB BC V1.2 is way much easier than type C PD ...

About what we have seen so far, the design looks interesting.
I didn't read all about on the forum, is there an official topic ?

I can't see any battery to fit inside. A supply with embbeded battery would be a killer. You can take your gadget in the meeting room, or on your desk without taking the heavy supply from the lab and wires ...
If you need, I made a custom size lipo this year with my team. I know how to do this, manufacturers, import, certifications, quality control, ... Plenty of work but worth every cents.

The dream would be with banana and usb output.
The usb output would be killer feature, it would able negociate with the sink. You choose what kind of charger/protocol you want test with your new fashion gadget.
Maybe you didn't thought about that. It's a free idea ;)

I hope see that in the shop soon. It's on my wishlist for santa. We all hope more videos about your work on this project.

Thanks for reading the comments Dave ;)
 

Offline MiroS

  • Regular Contributor
  • *
  • Posts: 175
  • Country: pl
Re: uSupply Firmware Released
« Reply #41 on: May 29, 2020, 10:14:05 am »
You linked in an ancient project coded in FORTRAN (I only know this because I actually searched for the SPICE wiki). I can't possibly comment on a programming language which I don't know and have never used. If the language does allow for splitting the code into multiple files, then 18k lines in a single file is in fact terrible coding practice.

It is not matter of used language. That FORTRAN pice of code is still in use and formed the foundation for modern electronics, so there is nothing to comment on this from your end, but there is a lot to learn from for  everyone who is going to write the code  - how to write it  to let others use it , modify , improve , document ,  enapsculate ,  reuse ,  maintain  ...




 

Offline diyaudio

  • Frequent Contributor
  • **
  • !
  • Posts: 683
  • Country: za
Re: uSupply Firmware Released
« Reply #42 on: May 29, 2020, 12:13:16 pm »
Dave and David thank you for contributing to the open source initiative, I learned a few things I didn't know about CMAkE could do like the integration with visual studio code which is my Editor/IDE of choice.

 :-+
 

Offline wizard69

  • Super Contributor
  • ***
  • Posts: 1184
  • Country: us
Re: uSupply Firmware Released
« Reply #43 on: May 29, 2020, 05:34:06 pm »
Code: [Select]
	const char * volatile current{
[]() -> const char *
{
I'm pretty sure it is Klingon. Live long and prosper.

One of the reasons for my love/hate of C++ is this very cryptic syntax if you are not involved in C++ full time.    We need something better than C but what that might be is unknown at the moment.    I actually think Apples Swift could have a lot of potential once it is fully fledged out as a language.

The problem with C++ is that you have to mentally parse every statement instead of simply reading it.    I can write something in Python and read tte code 5 years latter, write something in C++ and you may spend a considerable amount of time just figuring out what is going on even if it was written 5 weeks ago.   Of course with discipline C++ can be made idiomatic but that means leaving modern features behind.
 

Offline wizard69

  • Super Contributor
  • ***
  • Posts: 1184
  • Country: us
Re: uSupply Firmware Released
« Reply #44 on: May 29, 2020, 05:46:51 pm »
And yes, this C++ heavy code is very hard to read, and the whole functionality could be implemented in a much-much smaller code.

Not a comparison because the features aren't even remotely the same, but for those interested, previous versions of my uSupply written by me are 389 lines and 479 lines for two of the design variants.
That's everything, including comments, UI code, control code etc.
Not sure I'd like to guess how many lines this version of the uSupply would have taken me in "Dave C", but yes, it would been vastly different to the way David writes his code. I just let him do it the way he wanted, we have vastly different styles. But he writes games engines in his spare time for fun, and I wouldn't know C++ if it bit me on the arse.

I think most people here are pointing out the general difficulty one can have in reading somebody else's C++ code.   Dave may have a good solution here but for a third party reading the code it might take hours to understand the structure.    That is just one of the issues with C++, but I do wonder how long it would take Dave to grasp this code 5 years down the road with no refresh in between.

In the end the only thing the code needs to do is work correctly.   The good code organization is a positive indicator here so I'm not worried.    Frankly I'm waiting for you guys to ship.
 

Offline wizard69

  • Super Contributor
  • ***
  • Posts: 1184
  • Country: us
Re: uSupply Firmware Released
« Reply #45 on: May 29, 2020, 05:56:07 pm »
There is obviously a need for things like erratas and minor notes. There is no need to write poems, or a typical copy-paste with boiler-plate description of each parameter.

If the code implements some non-standard algorithm, the comments may help too. Especially on a way some constants are derived and stuff like that.

Peripheral drivers rarely need comments. In most cases the first thing I do if I really need to understand some code, I remove all the comments.

If you get no value out of the comments then they really don't server a purpose.   However that doesn't mean comments shouldn't be used.   The trick is to use comments in a way that facilitates understanding the code.   In many ways comments should be like foot notes in a formal paper that clarify or offer reference for the programmer to further explore.   Of course formal papers are not perfect but if programmers could grasp when more information is need we could go a long way to useful and not excessive comments.
 

Offline Dave

  • Super Contributor
  • ***
  • Posts: 1356
  • Country: si
  • I like to measure things.
Re: uSupply Firmware Released
« Reply #46 on: May 30, 2020, 08:54:31 pm »
It is not matter of used language. That FORTRAN pice of code is still in use and formed the foundation for modern electronics, so there is nothing to comment on this from your end, but there is a lot to learn from for  everyone who is going to write the code  - how to write it  to let others use it , modify , improve , document ,  enapsculate ,  reuse ,  maintain  ...
That spice2 project is most certainly not a good example how to write code. 18k lines in a single document is ridiculous.
Have a look at spice3. It's written in C and the whole project consists of dozens of documents instead of a handful of massive ones.
<fellbuendel> it's arduino, you're not supposed to know anything about what you're doing
<fellbuendel> if you knew, you wouldn't be using it
 

Online splin

  • Frequent Contributor
  • **
  • Posts: 999
  • Country: gb
Re: uSupply Firmware Released
« Reply #47 on: June 02, 2020, 01:34:56 am »

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'

Quote
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: [Select]
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.
Code: [Select]
/* 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);
Code: [Select]
/* 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:
Code: [Select]
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:
Code: [Select]
#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, &current_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....

Quote
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...
« Last Edit: June 02, 2020, 11:24:07 am by splin »
 
The following users thanked this post: Dave, uer166

Offline nuclearcat

  • Supporter
  • ****
  • Posts: 382
  • Country: lb
Re: uSupply Firmware Released
« Reply #48 on: June 02, 2020, 06:49:27 am »
Management, MISRA, few bytes of precious SRAM?
Did i missed something or we are talking about PoC code for uSupply (low cost power supply) made by <2 person (because eventually Dave2 not working on it anymore)?
 
The following users thanked this post: prasimix

Online splin

  • Frequent Contributor
  • **
  • Posts: 999
  • Country: gb
Re: uSupply Firmware Released
« Reply #49 on: June 02, 2020, 11:37:04 am »
Management, MISRA, few bytes of precious SRAM?
Did i missed something or we are talking about PoC code for uSupply (low cost power supply) made by <2 person (because eventually Dave2 not working on it anymore)?

I was referring to a hypothetical example of a large embedded development, written in C,  that happened to be using the same type of filter to illustrate some of the issues that can arise from a well meant attempt to 'improve' the project by providing library functions for commonly used functions.
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf