Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement high frequency reads for ADC and Digital In #421

Open
wants to merge 22 commits into
base: dev
Choose a base branch
from

Conversation

EarthenSky
Copy link

@EarthenSky EarthenSky commented May 17, 2024

The goal of this PR was to improve the performance of the SDK's ADC reads to be higher than 100hz. All 8 ADC inputs can consistently be read in less than 10ms on average (with a somewhat surprisingly high variance, likely due to the OS), while all 8 digital inputs can be read in less than 1ms on average.

It's possible to improve the performance more than 100 hz for all 8 ADC, however 1/3 of all time is being spend sleeping, while another 1/3 is spent transferring data over SPI. I think it's unlikely to get faster than 200 hz.

The speed improvements are due to bit optimizations, minimizing data sent over the channel, python optimizations, and batching of requests (minimize system calls opening & closing devices), all of which were found via profiling. For more info, see this (private) page in the wiki.

@@ -27,8 +26,7 @@ def _format_mux_values(mux_p: CH, mux_n: CH):

return mux_p_val, mux_n_val, mask


def generate_mux_opcodes(mux_updates: dict):
def generate_mux_opcodes(adc1_reg, adc1_mux, adc2_reg, adc2_mux):
Copy link
Author

@EarthenSky EarthenSky May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pack() is a surprisingly slow function, so replacing it improved performance a lot. The logging and dictionary operations also slowed this function down a reasonable amount, which is why I removed them. I tried to avoid removing logging statements where possible, however they did have an effect on the performance.

f"query_state: query_property='{adc_property}',"
" adc_property_bits={hex(adc_property_bits)},"
f" adc_property_value='{adc_property_value}'"
"query_state: query_property='{}',".format(adc_property) +
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format strings f"{...}" are eager, and so slow things down more. I opted to switch to lazy formatting where possible

Comment on lines 82 to 85
# pylint: disable=protected-access
gpio._line = pin_num
# pylint: disable=protected-access
gpio._reopen(pin_dir, edge="none", bias=pin_bias, drive="default", inverted=False)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change enables the batching of gpio reads, which minimizes the number of device open calls (I/O), which improves performance quite a bit. One tradeoff is that we touch private variables & functions, which could change if the library is updated. It seems the file these variables are in hasn't been modified for a year, but still something to keep in mind.

# NOTE: disabling register update validation because there's no reason for us to suspect the other
# values would change? I'm not sure why this was done before.
# This is also quite slow, and affects the performance significantly
#__validate_register_updates(original_regs, register_values)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this validation function, combined with the deepcopy above is quite slow, and I can't figure out if it actually does anything. If it does, I'll probably want to make a function unsafe_apply_opcodes() which removes it, so that the rest of the codebase isn't adversely affected.

@@ -980,6 +975,127 @@ def set_config(
[None])
self.__config(**args)

def adc1_config_and_read_samples_batch(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is massive because it is a combination of set_config and read_sample, but also most of the child functions. One issue with this codebase is that get_state is called a large number of times, even when only a few properties are needed. The construction of the ADCState object, and sometimes reading from the hardware is quite slow. By combining all the functions, we can read the register state from the hardware just once, then reuse those values & update our local copy after we complete all the hardware writes.

command_tup, register_values = create_commands(-1, register_values, mux_p, mux_n)
command_tup_list += [command_tup]

data_list = self.spi_apply_adc_commands(command_tup_list)
Copy link
Author

@EarthenSky EarthenSky May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we apply all the adc config. start, and read commands at once

@EarthenSky EarthenSky marked this pull request as ready for review May 17, 2024 22:20
@EarthenSky EarthenSky changed the base branch from main to dev May 17, 2024 22:20
sjpark608
sjpark608 previously approved these changes May 27, 2024
src/edgepi/adc/adc_multiplexers.py Outdated Show resolved Hide resolved
Copy link

Code Coverage

Package Line Rate Branch Rate Health
adc 91% 86%
calibration 100% 100%
dac 100% 97%
digital_input 100% 100%
digital_output 98% 95%
eeprom 83% 63%
eeprom.protobuf_assets.eeprom_data_classes 97% 52%
eeprom.protobuf_assets.generated_pb2 77% 50%
gpio 98% 94%
led 0% 0%
peripherals 69% 23%
pwm 96% 95%
reg_helper 93% 70%
relay 100% 100%
tc 98% 98%
utilities 100% 100%
Summary 92% (3196 / 3488) 80% (694 / 866)

Copy link
Collaborator

@sjpark608 sjpark608 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR could've been separate PRs, ADC, GPIO, and Digitial inputs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants