Severson-Group / AMDC-Firmware

Embedded system code (C and Verilog) which runs the AMDC Hardware
http://docs.amdc.dev/firmware
BSD 3-Clause "New" or "Revised" License
30 stars 5 forks source link

Change the maximum number of samples to reduce the data logging runtime #257

Closed Zhouzhou377 closed 2 years ago

Zhouzhou377 commented 2 years ago

When the runtime of the control loop + data logging is longer than 100 us, the data logging function can interrupt the control code. The machine will behave strangely, while the data are transferring from the AMDC to the PC.

Nathan helped to address this issue. The following changes can resolve this problem:

In file ./app-cpu1/common/sys/log.c, change max_num_samples = 100; to max_num_samples = 10;

npetersen2 commented 2 years ago

Thanks @Zhouzhou377 .

For background: this issue was introduced in the v1.0.0 release when dumping the logged sample data over Ethernet was added. The max_num_samples to which @Zhouzhou377 is referring is the total number of data samples the control core tries to send out each time slice (i.e. each 100us). For each sample, it takes a bit of code to package up the sample into a packet and send it out. If the ethernet Tx queues are full, it stops early (i.e. before 100 samples) and waits to try again until the next time slice.

We found that it took the Ethernet logging dump task about 50us average to run each time slice when the max_num_samples was 100. At 10, the mean run-time reduced to about 10us or so (can't quite remember exactly how much), which solved @Zhouzhou377 's issue.

This is a "hack" since it artificially limits the dump rate, but due to the cooperative nature of the firmware design (as of now), this is actually a reasonable solution. Note that, if the user stops their control while dumping data, this issue goes away... It is only when the user is trying to control their system AND dump at the same time over Ethernet AND their control takes >= ~50usec that is issue appears

I think we can increase the default max_num_samples to 20 and be fine for most people's control. I'll update the code and get this into the next bug fix release.

npetersen2 commented 2 years ago

max_num_samples is defined here: https://github.com/Severson-Group/AMDC-Firmware/blob/develop/sdk/app_cpu1/common/sys/log.c#L527