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

New radio protocol using protobuf #127

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kylestach
Copy link
Contributor

Experimental and untested but if we can support the bandwidth and memory (spoiler: we almost certainly can) this should make complex data interchange and robot configuration a lot easier.

This does allocate a big chunk of memory (16KiB right now) for the buffer; this can be trimmed down once we figure out better bounds for message sizes.

rj_radio_RobotCommand robot_command_proto;
};
// Block off a huge chunk of memory. It's really only this big because of (optional) log messages.
static std::array<uint8_t, 16 * 1024> buffer{};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buffer memory, for visibility. The RobotStatus struct isn't small either (~5KiB), just because of the potential existence of log messages. Note that that does not mean that the wire size is 5KiB, but the in-memory version statically allocates the maximum size of 256B of data per log message * 16 log messages (intentional feature, avoids dynamic malloc).

// Keep these variables in static memory. We'd rather get link errors if they're too big.
// We only ever use one of these at a time
static union {
rj_radio_RobotStatus robot_status_proto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

5828 bytes

// We only ever use one of these at a time
static union {
rj_radio_RobotStatus robot_status_proto;
rj_radio_RobotCommand robot_command_proto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

69 bytes (nice)

frame.gyro_z = imuDataLock->omegas[2];
frame.accel_x = imuDataLock->accelerations[0];
frame.accel_y = imuDataLock->accelerations[1];
auto motion_command = motionCommand.lock().value();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in this file do not completely fill in the new debug frame format. They were mostly intended to fix the fact that we were holding waaay too many locks throughout this whole function.

Later work will involve actually populating the new data (and providing logging facilities 😄 )

required LogLevel level = 1;
required uint32 timestamp_ms = 2;
optional string subsystem = 3 [(nanopb).max_size = 16];
required string message = 4 [(nanopb).max_size = 256];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This max size and the max count (16) listed below are up for discussion. These values make the in-memory struct huge (the wire version will cut out space though).

@Arvind-Srinivasan Arvind-Srinivasan removed their request for review July 5, 2021 04:40
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.

2 participants