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

feat(sensors): add a new can message to batch read sensor data #805

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ryanthecoder
Copy link
Contributor

In a first step to streamline tool_sensors.py we want to batch read sensor data, this will speed up the process and let us stream alot better

Copy link
Contributor

@caila-marashaj caila-marashaj left a comment

Choose a reason for hiding this comment

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

logic looks good to me!

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Couple small readability and robustness changes and also definitely want some tests for the index math and logic

.data_length = j,
.sensor_data = data,
};
can_client.send_can_message(can::ids::NodeId::host, response);
Copy link
Member

Choose a reason for hiding this comment

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

We haven't faced it before as much, but I think we can improve this a lot by changing (or adding a new version of) send_can_message that will either (a) block (by sleeping) or (b) return false when the local buffer is full. That will let us dynamically slow down data output when the buffer is full while not adding a delay all the time.

// slow it down so the can buffer doesn't choke
vtask_hardware_delay(20);
uint8_t j = 0;
while (j < 14 and i < count) {
Copy link
Member

Choose a reason for hiding this comment

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

We absolutely need some tests for the fiddly index math.

Copy link
Member

Choose a reason for hiding this comment

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

also can use j < data.size() or even take this to a range-for that stops early a la

for (auto& copy_to : data) {
    if (i >= count) {
        break;
    }
    copy_to = mmr920::reading_to_fixed_point(sensor_buffer->at(current_index));
    i++;
    current_index = (i + start) % static_cast<int>(SENSOR_BUFFER_SIZE);
}

for (int i = 0; i < count; i++) {
std::array<uint32_t, 14> data{};
int i = 0;
while (i < count) {
Copy link
Member

Choose a reason for hiding this comment

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

could make this for(size_t i = 0; i < count; ) { } if we wanted. in general, seems like we could use a size here instead of a signed value.

@@ -315,24 +315,32 @@ class MMR920 {
can_client.send_can_message(
can::ids::NodeId::host,
can::messages::Acknowledgment{.message_index = count});
for (int i = 0; i < count; i++) {
std::array<uint32_t, 14> data{};
Copy link
Member

Choose a reason for hiding this comment

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

would love to see this 14 derived from somewhere

@ryanthecoder ryanthecoder marked this pull request as draft September 30, 2024 18:40
@ryanthecoder
Copy link
Contributor Author

Marking as draft since Caila didn't get a chance to use this before her trip. I'll clean it up and make it better styled

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

Successfully merging this pull request may close these issues.

3 participants