From 692e9239fa1ed8dbf1b146d81a190e837c05f6ba Mon Sep 17 00:00:00 2001 From: Oskar Weigl Date: Sat, 28 Oct 2017 21:06:06 -0700 Subject: [PATCH] add comments --- Firmware/MotorControl/commands.cpp | 7 ++- Firmware/MotorControl/protocol.cpp | 5 ++ Firmware/MotorControl/protocol.hpp | 58 ++++++++++++++----- Firmware/Src/usbd_cdc_if.c | 4 ++ tools/odrive/protocol.py | 4 +- tools/odrive/usbbulk.py | 89 ++++++++++++++---------------- tools/odrive/util.py | 11 +++- 7 files changed, 111 insertions(+), 67 deletions(-) diff --git a/Firmware/MotorControl/commands.cpp b/Firmware/MotorControl/commands.cpp index 5cb57341..10eb6a71 100644 --- a/Firmware/MotorControl/commands.cpp +++ b/Firmware/MotorControl/commands.cpp @@ -37,9 +37,11 @@ static const GpioMode_t gpio_mode = GPIO_MODE_UART; //GPIO 1,2 is UART Tx,Rx /* Private variables ---------------------------------------------------------*/ /* Variables exposed to USB & UART via read/write commands */ +//Oskar: what is range information? // TODO: include range information in JSON description // clang-format off +//Oskar: The endpoint table should be const, yeah? Then it can be put in RO memory (flash). Endpoint endpoints[] = { Endpoint("vbus_voltage", const_cast(&vbus_voltage)), Endpoint("elec_rad_per_enc", const_cast(&elec_rad_per_enc)), @@ -57,6 +59,9 @@ constexpr size_t NUM_ENDPOINTS = sizeof(endpoints) / sizeof(endpoints[0]); // We could theoretically implement the USB channel as a packet based channel, // but on some platforms there's no direct USB endpoint access, so the device // should better just behave like a serial device. + +//Oskar: We should discuss this, possibly have both options. +// On windows the serial driver seems really buggy... class USBSender : public StreamWriter { public: int write_bytes(const uint8_t* buffer, size_t length) { @@ -174,7 +179,7 @@ void communication_task(void const * argument) { void USB_receive_packet(const uint8_t *buffer, size_t length) { //printf("[USB] got %d bytes, first is %c\r\n", length, buffer[0]); osDelay(5); #ifdef ENABLE_LEGACY_PROTOCOL - const uint8_t *legacy_commands = (const uint8_t*)"pvcgsmo"; + const uint8_t* legacy_commands = (const uint8_t*)"pvcgsmo"; while (*legacy_commands && length) { if (buffer[0] == *(legacy_commands++)) { //printf("[USB] process legacy command %c\r\n", buffer[0]); osDelay(5); diff --git a/Firmware/MotorControl/protocol.cpp b/Firmware/MotorControl/protocol.cpp index bc118812..3fb1c3d4 100644 --- a/Firmware/MotorControl/protocol.cpp +++ b/Firmware/MotorControl/protocol.cpp @@ -16,6 +16,7 @@ /* Private variables ---------------------------------------------------------*/ // The order in this list must correspond to the order in EndpointTypeID_t +//Oskar: Isn't it better to then have an array of tuples(or structs), so that they are always paired at definition? const char *type_names_[] = { "json", "int32[]", @@ -64,6 +65,7 @@ void write_buffer(const uint8_t* input, size_t input_length, size_t* skip, uint8 } } +//Oskar: consier a JsonWriter object (as per slack discussion) static void write_string(const char* str, size_t* skip, uint8_t** output, size_t* output_length) { write_buffer(reinterpret_cast(str), strlen(str), skip, output, output_length); } @@ -162,6 +164,8 @@ int PacketToStreamConverter::write_packet(const uint8_t *buffer, size_t length) // Calculates the CRC16 of the JSON interface descriptor. // Make sure this stays consistent with what interface_query returns. // The init value is the protocol version. + +//Oskar: consider non-loop version of calc_crc16 in write_buffer, as per slack discussion uint16_t BidirectionalPacketBasedChannel::calculate_json_crc16(void) { uint8_t buffer[64]; size_t offset = 0; @@ -206,6 +210,7 @@ void BidirectionalPacketBasedChannel::interface_query(const uint8_t* input, size write_string("]", &offset, &output, output_length); } +//Oskar: Can you please make a google sheet which describes the packet layout/format int BidirectionalPacketBasedChannel::write_packet(const uint8_t* buffer, size_t length) { //printf("got packet of length %d: \r\n", length); osDelay(5); hexdump(buffer, length); if (length < 4) diff --git a/Firmware/MotorControl/protocol.hpp b/Firmware/MotorControl/protocol.hpp index 2cb9d064..69a9dc25 100644 --- a/Firmware/MotorControl/protocol.hpp +++ b/Firmware/MotorControl/protocol.hpp @@ -2,9 +2,12 @@ * # ODrive Communication Protocol # * * Communicating with an ODrive consists of a series of endpoint operations. -* operations. An endpoint is usually a single number or string. +* An endpoint can be any data representation that can be serialized. +* There is a default seralization implementation for POD types; for custom types +* you must (de)seralize yourself. In the future we may provide a default seralizer +* for stucts. * The available endpoints can be enumerated by reading the JSON from endpoint 0 -* and can theoretically be different for each channel (they are not in practice). +* and can theoretically be different for each communication interface (they are not in practice). * * Each endpoint operation can send bytes to one endpoint (referenced by it's ID) * and at the same time receive bytes from the same endpoint. The semantics of @@ -19,13 +22,18 @@ * ## Stream format: ## * (For instance UART) * -* 1. sync-byte -* 2. packet-length (0-127, larger values are reserved) -* 3. crc8(sync-byte + packet-length) -* 4. packet +* 1. sync byte +* 2. packet length (0-127, larger values are reserved) +* 3. crc8(sync byte + packet length) +* 4. packet (as per below) * * ## Packet format: ## * (For instance USB) + +//Oskar: All packet formats I anticipate (USB, CAN, Ethernet) already have a transport +// level CRC. So maybe we can throw out the packet level CRC check, and instead add +// a data crc as step 5 on the stream format? + * * __Request__ * @@ -96,16 +104,16 @@ inline size_t write_le(float value, uint8_t* buffer) { template<> inline size_t read_le(uint16_t* value, const uint8_t* buffer) { *value = (static_cast(buffer[0]) << 0) | - (static_cast(buffer[1]) << 8); + (static_cast(buffer[1]) << 8); return 2; } template<> inline size_t read_le(uint32_t* value, const uint8_t* buffer) { *value = (static_cast(buffer[0]) << 0) | - (static_cast(buffer[1]) << 8) | - (static_cast(buffer[2]) << 16) | - (static_cast(buffer[3]) << 24); + (static_cast(buffer[1]) << 8) | + (static_cast(buffer[2]) << 16) | + (static_cast(buffer[3]) << 24); return 4; } @@ -121,6 +129,10 @@ template static inline T read_le(const uint8_t** buffer, size_t* length) { T result; size_t cnt = read_le(&result, *buffer); + //Oskar: Is the style where you have a mutable length and buffer pointer + // a common form? I don't think I have seen it before: let me know if you have some + // reference of this style. + // Maybe using iterators is better? *buffer += cnt; *length -= cnt; return result; @@ -153,6 +165,9 @@ typedef enum { // @param output_length: pointer to the remaining length of the output buffer. // The handler must update this value to subtract the number of written bytes. // The pointer itself is guaranteed not to be NULL. + +//Oskar: Suggestion: return number of bytes written, and pass output_length by value. +// This style is more consistent with stdio functions typedef std::function EndpointHandler; @@ -161,10 +176,12 @@ void default_read_endpoint_handler(void* ctx, const uint8_t* input, size_t input const T* value = reinterpret_cast(ctx); // If the old value was requested, call the corresponding little endian serialization function if (*output_length) { - uint8_t buffer[8]; // TODO: make buffer size dependent on the type + // uint8_t buffer[8]; // TODO: make buffer size dependent on the type + uint8_t buffer[sizeof(T)]; //Oskar: You can do this. size_t cnt = write_le(*value, buffer); if (cnt > *output_length) - cnt = *output_length; + cnt = *output_length; //Oskar: I woudldn't clip like this, some types become + // very wrong when clipped little endian (like float). Just write nothing if it doesnt fit. memcpy(output, buffer, cnt); *output_length -= cnt; } @@ -174,7 +191,7 @@ template void default_readwrite_endpoint_handler(void* ctx, const uint8_t* input, size_t input_length, uint8_t* output, size_t* output_length) { T* value = reinterpret_cast(ctx); - // Call the handler for the const version of the endpoint's type - i.e. read the endpoint value into output + // Read the endpoint value into output default_read_endpoint_handler(ctx, input, input_length, output, output_length); // If a new value was passed, call the corresponding little endian deserialization function @@ -182,6 +199,7 @@ void default_readwrite_endpoint_handler(void* ctx, const uint8_t* input, size_t uint8_t buffer[8] = { 0 }; // TODO: make buffer size dependent on the type if (input_length > sizeof(buffer)) input_length = sizeof(buffer); + //Oskar: Why do we need to take a copy of the input buffer? memcpy(buffer, input, input_length); // TODO: abort if not enough bytes received read_le(value, buffer); } @@ -225,10 +243,15 @@ private: void* const ctx_; }; +//Oskar: Writer implies that it writes things, but that's not what this is; +// you can write to it, but it doesn't write things. +// This interface should either be a "handler", "processor" or "reader" since that's what it does to the +// stuff you give to it, or "writable", since you can write to it. class PacketWriter { public: // @brief Processes a packet. + //Oskar: What is the return value? // TODO: define what happens when the output is congested. We can either drop the data or block. // TODO: define what happens when the packet is larger than what the implementation can handle. virtual int write_packet(const uint8_t* buffer, size_t length) = 0; @@ -238,6 +261,7 @@ public: class StreamWriter { public: // @brief Processes a chunk of bytes that is part of a continuous stream. + //Oskar: What is the return value? // TODO: define what happens when the output is congested. We can either drop the data or block. virtual int write_bytes(const uint8_t* buffer, size_t length) = 0; }; @@ -255,7 +279,7 @@ public: private: uint8_t header_buffer_[3]; size_t header_index_ = 0; - uint8_t packet_buffer_[128]; + uint8_t packet_buffer_[128]; //Oskar: Use some constexpr config name, hardcoded numbers are not so nice size_t packet_index_ = 0; size_t packet_length_ = 0; PacketWriter& output_; @@ -302,6 +326,7 @@ private: } //ReceiveCallback c = BidirectionalPacketBasedChannel::receive_fetch_request_ex; + //Oskar: by channel_specific, do you mean protocol_internal/specific? Endpoint channel_specific_endpoints_[2] = { Endpoint("", AS_JSON, BidirectionalPacketBasedChannel::interface_query_handler, nullptr, this), Endpoint("subscriptions", AS_INT32_ARRAY, BidirectionalPacketBasedChannel::subscription_handler, nullptr, this) @@ -312,6 +337,7 @@ private: size_t n_endpoints_; PacketWriter& output_; + Endpoint* get_endpoint(size_t index) { if (index < NUM_CHANNEL_SPECIFIC_ENDPOINTS){ return &channel_specific_endpoints_[index]; @@ -322,11 +348,15 @@ private: } } + //Oskar: What is a subscription? Let's discuss on slack void subscription(const uint8_t* input, size_t input_length, uint8_t* output, size_t* output_length) { // TODO: handle return; } + //Oskar: try to keep a consistent declaration ordering between functions and data. + // see: https://google.github.io/styleguide/cppguide.html#Declaration_Order + size_t expected_seq_no_ = 0; uint8_t tx_buf_[TX_BUF_SIZE]; const uint16_t json_crc_; diff --git a/Firmware/Src/usbd_cdc_if.c b/Firmware/Src/usbd_cdc_if.c index 65a3b452..48c7151f 100644 --- a/Firmware/Src/usbd_cdc_if.c +++ b/Firmware/Src/usbd_cdc_if.c @@ -277,6 +277,10 @@ static int8_t CDC_Receive_FS (uint8_t* Buf, uint32_t *Len) USBRxBufferLen = *Len; osSemaphoreRelease(sem_usb_irq); + //Oskar: You shouldn't return from this function until you have copied data out of Buf, + // see the @note above. + // So I would revert to the tread-deffered USB irq processing, and call USB_receive_packet from here. + // once the data is handled, the processing thread will start the next transmission return (USBD_OK); /* USER CODE END 6 */ diff --git a/tools/odrive/protocol.py b/tools/odrive/protocol.py index 047b65d9..ef083580 100644 --- a/tools/odrive/protocol.py +++ b/tools/odrive/protocol.py @@ -11,6 +11,7 @@ PROTOCOL_VERSION = 1 CRC8_DEFAULT = 0x37 # this must match the polynomial in the C++ implementation CRC16_DEFAULT = 0x3d65 # this must match the polynomial in the C++ implementation +#Oskar: There must be a crc library for python already? def calc_crc(remainder, value, polynomial, bitwidth): topbit = (1 << (bitwidth - 1)) @@ -107,6 +108,7 @@ class StreamToPacketConverter(StreamWriter): self._packet_length = 0 if isinstance(result, Exception): + #Oskar: why are we removing exception information? Just let the original exception go up? raise Exception("something went wrong") @@ -116,7 +118,7 @@ class PacketToStreamConverter(PacketWriter): def write_packet(self, packet): if (len(packet) >= 128): - raise Exception("packet larger than 127 currently not supported") + raise NotImplementedError("packet larger than 127 currently not supported") header = [SYNC_BYTE, len(packet)] header.append(calc_crc8(CRC8_INIT, header)) diff --git a/tools/odrive/usbbulk.py b/tools/odrive/usbbulk.py index d5f18e9a..5bbeb693 100644 --- a/tools/odrive/usbbulk.py +++ b/tools/odrive/usbbulk.py @@ -12,6 +12,11 @@ def noprint(x): # Even though USB is packet based, we do stream based communication because some # systems don't allow direct access to the USB endpoints, in which case the # device has to behave like a serial device. + +#Oskar: If we are using stream based comms over USB, we shouldn't be detach_kernel_driver and epr.read, +# we should be using /dev/ttyACM0 with system read/writes. +# Actually, we should discuss the approach here, we need to decide if we should do packet based or not. + class USBBulkDevice(odrive.protocol.StreamReader, odrive.protocol.StreamWriter): def __init__(self, dev, printer=noprint): self.dev = dev @@ -32,62 +37,50 @@ class USBBulkDevice(odrive.protocol.StreamReader, odrive.protocol.StreamWriter): return string def init(self, printer=noprint): + # detach kernel driver try: - # detach kernel driver - try: - if self.dev.is_kernel_driver_active(1): - self.dev.detach_kernel_driver(1) - printer("Detached Kernel Driver\n") - except NotImplementedError: - pass #is_kernel_driver_active not implemented on Windows - # set the active configuration. With no arguments, the first - # configuration will be the active one - self.dev.set_configuration() - # get an endpoint instance - self.cfg = self.dev.get_active_configuration() - self.intf = self.cfg[(1,0)] - # write endpoint - self.epw = usb.util.find_descriptor(self.intf, - # match the first OUT endpoint - custom_match = \ - lambda e: \ - usb.util.endpoint_direction(e.bEndpointAddress) == \ - usb.util.ENDPOINT_OUT - ) - assert self.epw is not None - printer("EndpointAddress for writing {}\n".format(self.epw.bEndpointAddress)) - # read endpoint - self.epr = usb.util.find_descriptor(self.intf, - # match the first IN endpoint - custom_match = \ - lambda e: \ - usb.util.endpoint_direction(e.bEndpointAddress) == \ - usb.util.ENDPOINT_IN - ) - assert self.epr is not None - printer("EndpointAddress for reading {}\n".format(self.epr.bEndpointAddress)) - except usb.core.USBError: - #return -1 - raise + if self.dev.is_kernel_driver_active(1): + self.dev.detach_kernel_driver(1) + printer("Detached Kernel Driver\n") + except NotImplementedError: + pass #is_kernel_driver_active not implemented on Windows + # set the active configuration. With no arguments, the first + # configuration will be the active one + self.dev.set_configuration() + # get an endpoint instance + self.cfg = self.dev.get_active_configuration() + self.intf = self.cfg[(1,0)] + # write endpoint + self.epw = usb.util.find_descriptor(self.intf, + # match the first OUT endpoint + custom_match = \ + lambda e: \ + usb.util.endpoint_direction(e.bEndpointAddress) == \ + usb.util.ENDPOINT_OUT + ) + assert self.epw is not None + printer("EndpointAddress for writing {}\n".format(self.epw.bEndpointAddress)) + # read endpoint + self.epr = usb.util.find_descriptor(self.intf, + # match the first IN endpoint + custom_match = \ + lambda e: \ + usb.util.endpoint_direction(e.bEndpointAddress) == \ + usb.util.ENDPOINT_IN + ) + assert self.epr is not None + printer("EndpointAddress for reading {}\n".format(self.epr.bEndpointAddress)) def shutdown(self): return 0 def write_bytes(self, usbBuffer): - try: - ret = self.epw.write(usbBuffer, 0) - return ret - except usb.core.USBError: - #return -1 - raise + ret = self.epw.write(usbBuffer, 0) + return ret def read_bytes(self, bufferLen): - try: - ret = self.epr.read(bufferLen, 100) - return ret - except usb.core.USBError: - #return -1 - raise + ret = self.epr.read(bufferLen, 100) + return ret def send_max(self): return 64 diff --git a/tools/odrive/util.py b/tools/odrive/util.py index cd9d866c..10319c5d 100644 --- a/tools/odrive/util.py +++ b/tools/odrive/util.py @@ -9,10 +9,15 @@ class ODriveError(Exception): class ODriveNotConnectedError(ODriveError): pass -USB_DEV_ODRIVE_3_1 = (0x1209, 0x0D31) -USB_DEV_ODRIVE_3_3 = (0x1209, 0x0D33) +USB_DEV_ODRIVE_3_1 = (0x1209, 0x0D31) +USB_DEV_ODRIVE_3_2 = (0x1209, 0x0D32) +USB_DEV_ODRIVE_3_3 = (0x1209, 0x0D33) # all devices -USB_VID_PID_PAIRS = [USB_DEV_ODRIVE_3_1, USB_DEV_ODRIVE_3_3] +USB_VID_PID_PAIRS = [ + USB_DEV_ODRIVE_3_1, + USB_DEV_ODRIVE_3_2, + USB_DEV_ODRIVE_3_3, + ] def noprint(x): pass