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

Wrapped message rewrite #22

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions BURT_proto.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ class BurtProto {
static bool decodeRaw(const uint8_t* buffer, int length, const pb_msgdesc_t* fields, void* message);

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
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.

T result;
decodeRaw(buffer, length, fields, &result);
if(!decodeRaw(buffer, length, fields, &result))
return std::nullopt;
return result;
}
};
78 changes: 61 additions & 17 deletions BURT_serial.cpp
Original file line number Diff line number Diff line change
@@ -1,39 +1,73 @@
#include "BURT_serial.h"
#include "BURT_proto.h"
#include "version.pb.h"

BurtSerial::BurtSerial(Device device, ProtoHandler onMessage, const pb_msgdesc_t* descriptor, int length) :
BurtSerial::BurtSerial(Device device, ProtoHandler onMessage, const pb_msgdesc_t* descriptor, int length, Version version, bool receipt = false) :
device(device),
onMessage(onMessage),
descriptor(descriptor),
length(length)
length(length),
version(version),
receipt(receipt),
{ }

bool isResetCode(uint8_t* buffer, int length) {
return length >= 4
&& buffer[0] == 0
&& buffer[1] == 0
&& buffer[2] == 0
&& buffer[3] == 0;
}
// bool isResetCode(uint8_t* buffer, int length) {
// return length >= 4
// && buffer[0] == 0
// && buffer[1] == 0
// && buffer[2] == 0
// && buffer[3] == 0;
// }
Comment on lines +14 to +20
Copy link
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;
// }


void BurtSerial::update() {
int length = Serial.available();
if (length == 0) return;
uint8_t input[length];
int receivedLength = Serial.readBytes((char*) input, length);

if (!isConnected) {
if (!isConnected){
tryConnect(input, length);
} else if (isResetCode(input, receivedLength)) {
// This is our special "reset" code. Respond with 1111
uint8_t response[4] = {0x01, 0x01, 0x01, 0x01};
Serial.write(response, 4);
isConnected = false;
} else {
onMessage(input, length);
}

// NO CHECK
WrappedMessage msg = BurtProto::decode<WrappedMessage>(input, length, WrappedMessage_fields);

if(msg.version.major != this->version.major)
{

// Send back invalid version message?
}

switch(msg.type)
{
case MessageType::HEARTBEAT:
// check sender validity?
Copy link
Member

@Levi-Lesches Levi-Lesches Nov 12, 2024

Choose a reason for hiding this comment

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

Respond back!

break;
case MessageType::DISCONNECT:
uint8_t response[4] = {0x01, 0x01, 0x01, 0x01};
//Serial.write(response, 4);
BurtSerial::send(response)
isConnected = false;
break;
case MessageType::COMMAND:
// what special thing we do here other than onMessage(input,length) lil bro
Copy link
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.

// break;
default:
onMessage(input, length);
Comment on lines +55 to +56
Copy link
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.


}

// } else if (isResetCode(input, receivedLength)) {
// // This is our special "reset" code. Respond with 1111
// uint8_t response[4] = {0x01, 0x01, 0x01, 0x01};
// Serial.write(response, 4);
// isConnected = false;
// } else {
// onMessage(input, length);
// }
}

// CHANGE THIS
void BurtSerial::tryConnect(uint8_t* input, int length) {
// Parse as an incoming Connect request
Connect connect = BurtProto::decode<Connect>(input, length, Connect_fields);
Expand Down Expand Up @@ -63,6 +97,11 @@ void BurtSerial::tryConnect(uint8_t* input, int length) {
* @return Returns `true` if the entire message is sent successfully, `false` otherwise.
*/
bool BurtSerial::send(const void* message) {

// Wrap it to wrapped message
BurtProto::encode()
Comment on lines +101 to +102
Copy link
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.


//
if (!isConnected) return false;

uint8_t* buffer = new uint8_t[length];
Expand All @@ -72,3 +111,8 @@ bool BurtSerial::send(const void* message) {
delete[] buffer;
return encodedLength == sentLength;
}

bool BurtSerial:sendLogMessage(BurtLog message){

return true;
Comment on lines +116 to +117
Copy link
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

}
7 changes: 6 additions & 1 deletion BURT_serial.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,25 @@
#include <Arduino.h>

#include "BURT_proto.h"
#include "version.pb.h"

class BurtSerial {
public:
bool isConnected = false;

BurtSerial(Device device, ProtoHandler onMessage, const pb_msgdesc_t* descriptor, int length);
BurtSerial(Device device, ProtoHandler onMessage, const pb_msgdesc_t* descriptor, int length, Version version);
void setup() { /* No setup needed */ }
void update();
bool send(const void* message);
void decode();
bool sendLogMessage(BurtLog message);
bool receipt;

private:
void tryConnect(uint8_t* input, int length);
Device device;
ProtoHandler onMessage;
const pb_msgdesc_t* descriptor;
int length;
Version version;
};
2 changes: 1 addition & 1 deletion Protobuf
Submodule Protobuf updated 6 files
+10 −13 arm.proto
+12 −9 core.proto
+3 −1 drive.proto
+16 −0 utils.proto
+1 −1 version.proto
+19 −5 wrapper.proto
12 changes: 12 additions & 0 deletions version.pb.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/* Automatically generated nanopb constant definitions */
/* Generated by nanopb-0.4.9 */

#include "version.pb.h"
#if PB_PROTO_HEADER_VERSION != 40
#error Regenerate this file with the current version of nanopb generator.
#endif

PB_BIND(Version, Version, AUTO)



51 changes: 51 additions & 0 deletions version.pb.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/* Automatically generated nanopb header */
/* Generated by nanopb-0.4.9 */

#ifndef PB_VERSION_PB_H_INCLUDED
#define PB_VERSION_PB_H_INCLUDED
#include "pb.h"

#if PB_PROTO_HEADER_VERSION != 40
#error Regenerate this file with the current version of nanopb generator.
#endif

/* Struct definitions */
typedef struct _Version {
int32_t major;
int32_t minor;
} Version;


#ifdef __cplusplus
extern "C" {
#endif

/* Initializer values for message structs */
#define Version_init_default {0, 0}
#define Version_init_zero {0, 0}

/* Field tags (for use in manual encoding/decoding) */
#define Version_major_tag 1
#define Version_minor_tag 2

/* Struct field encoding specification for nanopb */
#define Version_FIELDLIST(X, a) \
X(a, STATIC, SINGULAR, INT32, major, 1) \
X(a, STATIC, SINGULAR, INT32, minor, 2)
#define Version_CALLBACK NULL
#define Version_DEFAULT NULL

extern const pb_msgdesc_t Version_msg;

/* Defines for backwards compatibility with code written before nanopb-0.4.0 */
#define Version_fields &Version_msg

/* Maximum encoded size of messages (where known) */
#define VERSION_PB_H_MAX_SIZE Version_size
#define Version_size 22

#ifdef __cplusplus
} /* extern "C" */
#endif

#endif