add comments

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<const float*>(&vbus_voltage)),
     Endpoint("elec_rad_per_enc", const_cast<const float*>(&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);

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<const uint8_t*>(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)

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>(float value, uint8_t* buffer) {
 template<>
 inline size_t read_le<uint16_t>(uint16_t* value, const uint8_t* buffer) {
     *value = (static_cast<uint16_t>(buffer[0]) << 0) |
-            (static_cast<uint16_t>(buffer[1]) << 8);
+             (static_cast<uint16_t>(buffer[1]) << 8);
     return 2;
 }
 
 template<>
 inline size_t read_le<uint32_t>(uint32_t* value, const uint8_t* buffer) {
     *value = (static_cast<uint32_t>(buffer[0]) << 0) |
-            (static_cast<uint32_t>(buffer[1]) << 8) |
-            (static_cast<uint32_t>(buffer[2]) << 16) |
-            (static_cast<uint32_t>(buffer[3]) << 24);
+             (static_cast<uint32_t>(buffer[1]) << 8) |
+             (static_cast<uint32_t>(buffer[2]) << 16) |
+             (static_cast<uint32_t>(buffer[3]) << 24);
     return 4;
 }
 
@@ -121,6 +129,10 @@ template<typename T>
 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<void(void* ctx, const uint8_t* input, size_t input_length, uint8_t* output, size_t* output_length)> EndpointHandler;
 
 
@@ -161,10 +176,12 @@ void default_read_endpoint_handler(void* ctx, const uint8_t* input, size_t input
     const T* value = reinterpret_cast<const T*>(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<T>(*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<typename T>
 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<T*>(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<T>(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<T>(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_;

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 */ 

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

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

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