Skip to content

Wrapped message rewrite#22

Draft
warfarm wants to merge 14 commits into
mainfrom
wrapped-message-rewrite
Draft

Wrapped message rewrite#22
warfarm wants to merge 14 commits into
mainfrom
wrapped-message-rewrite

Conversation

@warfarm

@warfarm warfarm commented Nov 12, 2024

Copy link
Copy Markdown

No description provided.

@Levi-Lesches Levi-Lesches left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good so far!

Comment thread BURT_proto.h Outdated
Comment thread BURT_proto.h

template<typename T>
static T decode(const uint8_t* buffer, int length, const pb_msgdesc_t* fields) {
static std::optional<T> decode(const uint8_t* buffer, int length, const pb_msgdesc_t* fields) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I support this and the move to safety in general, but let's be sure to follow up on all the firmware that we did actually check these values.

Comment thread BURT_serial.cpp
Comment on lines +11 to +17
// bool isResetCode(uint8_t* buffer, int length) {
// return length >= 4
// && buffer[0] == 0
// && buffer[1] == 0
// && buffer[2] == 0
// && buffer[3] == 0;
// }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// bool isResetCode(uint8_t* buffer, int length) {
// return length >= 4
// && buffer[0] == 0
// && buffer[1] == 0
// && buffer[2] == 0
// && buffer[3] == 0;
// }

Comment thread BURT_serial.cpp Outdated
Comment thread BURT_serial.cpp
switch(msg.type)
{
case MessageType::HEARTBEAT:
// check sender validity?

@Levi-Lesches Levi-Lesches Nov 12, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Respond back!

Comment thread BURT_serial.cpp Outdated
Comment thread BURT_serial.cpp Outdated
isConnected = false;
break;
case MessageType::COMMAND:
// what special thing we do here other than onMessage(input,length) lil bro

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do a version check first. If the major versions are the same, call onMessage.

Comment thread BURT_serial.cpp Outdated
Comment on lines +44 to +45
default:
onMessage(input, length);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's get rid of this for now then.

Comment thread BURT_serial.cpp Outdated
Comment on lines +90 to +91
// Wrap it to wrapped message
BurtProto::encode()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So you don't actually use encode() here, but rather first encode the message, then put it as the bytes of the wrapper, then encode the wrapper and send that.

...and it's now that I'm realizing this will cause you some trouble. Read this section of the docs for more background.

Comment thread BURT_serial.cpp
Comment on lines +105 to +106

return true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe delete this so that you get the warning until you're done. Or introduce this in a separate PR

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