ADC averaging code

After my noise issues in BP7 REV0 proto run - #176 by electronic_eel I now want to implement a generic adc averaging function that can be used by the v-command as well as some other functions, like the future impedance test code.

My current plan is to always calculate the base values in amux_sweep() and store the results in a global array similar to hw_adc_voltage[].

hw_adc_voltage[] is currently defined for each BP model in /src/platform/. There is a comment above the structure
//TODO: move all this nonsense to the system config

See for example here:

The hw_pin_voltage_ordered[] array currently looks like it is the same for all models, it is just the enums in the header that differ.

So before I go and duplicate all this for each model I want to ask how you envision a better solution?

I guess it must be some structure you can iterate in a loop and index like hw_pin_voltage_ordered[] is now. Just a block of defines wouldn’t work for some code cases I’v seen.

Maybe one common array hw_adc_voltage[] that the amux_sweep() function already stores in the correct order?

I’m not familiar enough with the codebase yet to know what the best place for this kind of config is and how much work moving it would entail.

Or should I just duplicate the logic and create a hw_pin_average_voltage_ordered[] that has the same kind of pointers like hw_pin_voltage_ordered[]?

1 Like

So the issue was routing. Direct routing at some point meant backwards IO pin measurements vs the order we use them. The main purpose is to have pointers in order that map to the terminal tool bar, v, and LCD in a for loop nicely.

That, and the bio2bufdir and bio2bufio, are zombie code from the very beginning of bring up. I’m unhappy with it, but it does work and while I have several ideas to improve upon it, it has not been a priority. Thus ā€œnonsenseā€.

1 Like

Yeah, that the pin assignments differ between the versions is normal and you need some way to map that to the logical order in the code.

Your answer sounds like I should just copy the current way for my averaging and add something like this to each platform file:

uint32_t* hw_pin_avgsum_voltage_ordered[]={
    &hw_adc_avgsum_voltage[HW_ADC_MUX_VREF_VOUT],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO0],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO1],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO2],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO3],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO4],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO5],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO6],
    &hw_adc_avgsum_voltage[HW_ADC_MUX_BPIO7]
};

correct?

1 Like

Sure, that should fine.

Are you planning to start measuring more than once every 0.5s in the core 1 loop, or is this for a separate intensive measurement mode?

The current sense already gets a bit of smoothing, but it still only runs every 0.5s

The reason I ask is you might be able to just average into the existing variable, and I can get rid of the nonsense and do something better.

got it, thanks.

My current idea was to add the averaging code to the amux_sweep() function. So whenever it is called, it keeps reading out the adc values as it currently does. I would add to always also update the averaging sum (one integer addition, one integer shift, one integer subtraction, so very lightweight).

When you want to use the average value, you call a (inline) helper function that does the correct calculation from this average sum.

So the amux_sweep() function would be called like 0.5s in the core 1 loop like it is now. But it is also already called more often from the other core in cases more timely updates are wanted, like the continous V command.

1 Like

I think you need both. At least unless we are going to measure all the pins each millisecond or so.

Because with the slowish frequency we have now, you need to wait a few seconds after a step change properly shows in the average. You don’t want that for example in the quick overview on the screen or the terminal.

So I’d keep the adc code as it is now and just add the average as an option. Completely overhauling the ADC concept is not something I had planned for tonight :wink:

1 Like

I don’t think that would be a bad feature to implement system wide. And apply to the amux sweep as is. Maybe with a sampling frequency update.

The ā€œnonsenseā€ is that some commands code does an amux sweep and then reads out the variable. There are also functions that can read a single pin and return the value. This is mostly an issue in the implementation of the w_psu command, but I am afkb and can’t be certain.

1 Like

The ā€œnonsenseā€ is that there is a busy_wait_us(60); within a loop over HW_ADC_MUX_COUNT in amux_sweep(). That prevents you from always having the amux sweeping in the background.

I don’t have an easy solution for this within your code concept. It is just that I see again why I prefer using an RTOS for projects I start from scratch. There this would be a separate thread and instead of a busy wait I’d just yield for 60µs and the RTOS would continue some other thread instead.

2 Likes

For sure!

I need to look at the code, but I believe some of that is settling time for the channel switch of the amux.

Yes, you need to wait for the mux to switch and settle afterwards.

This is a prime example of code where you need to do a little bit, wait, do the next little bit and so on. And the waiting doesn’t even need to be precise, it just must not be too short. So something that would map very well to RTOS threads but would be a hassle to manually implement in C.

A few friends of mine praise async rust for these patterns instead of threads. This would probably also work well for this, but I don’t have much experience with it and have never tried it on microcontrollers yet.

1 Like

Rust is adorable and there’s legitimately money available to fund rust development on certain platforms.

My issue with asynchronous languages, most experience with node, is they compile some synthetic language to a giant state machine. Id rather write my own state machines in a cooperative multitasking loop. With node I end up debugging the compiled typescript to JavaScript and it’s really unpleasant.

RTOS would be fine though, if that’s an unobtrusive solution.

Yeah, debugging a big block of async code can really be an issue. Especially on a microcontroller & realtime platform where you don’t have the resources to easily dump the state structures into a specialized debugger.

I have zero experience with node and javascript.

As an RTOS I really like ChibiOS. https://www.chibios.org
If you want to get an overview, see ChibiOS free embedded RTOS - ChibiOS/RT

But the issue for using it for the BusPirate would mostly be licensing. It is either GPL or commercial.

1 Like

Adding an RTOS would be a non-trivial and obtrusive change. :rofl:

I agree that it’s a pain to handle those types of ā€œdo state A steps, wait Y, do state B steps, ā€¦ā€. You have to pick your poison: Async programming, or the pain of writing state-based code manually.

Asynchronous programming languages wrap that complexity for you, and as you pointed out, that also makes it much more difficult to reason about the state machine they generate. When such async language is mature for a platform, the compilers can optimize things significantly better than people, and the people can rely on the results doing the right thing. When they are not mature, … well, historically it seems the platform becomes a bug-finding safari, only some of which are the fault of the user.

If you decide to move to a RTOS anyways, I’d recommend sticking with one(s) Raspberry Pi provides. Support, debugging, and platform-specific hacks will at least exist, and extensions and tools will be more readily available. FreeRTOS is one that I’ve seen commonly used, and seems to have reasonably mature code base, although I don’t know if that’s one of the ā€œblessedā€ ones in the SDK?

3 Likes

RasPi are using FreeRTOS themselves in their picoprobe code:

The base FreeRTOS kernel is MIT-licensed, so no licensing issues integrating it.

I have no experience with FreeRTOS yet and can’t tell how much work integrating it would be and how it compares to the features I’m used to on ChibiOS.

2 Likes

I created a pull request for my averaging code:

If this is ok, please also consider cherry-picking into the bp7r0 branch so that I can easily use the code there without causing conflicts further down the road.

1 Like

I will get this done first thing tomorrow.

1 Like

Merged into main and 7R0.

1 Like

Thank you for merging.

1 Like