add comments

.vscode/launch.json

@@ -5,16 +5,12 @@
     "version": "0.2.0",
     "configurations": [
         {
-            "name": "Debug Python Utility",
+            "name": "Python",
             "type": "python",
             "request": "launch",
             "stopOnEntry": true,
             "pythonPath": "${config:python.pythonPath}",
-            "program": "${workspaceRoot}/tools/test_communication.py",
-            "args": [
-                // add specific arguments that you might wanna test
-                //"--serial", "/dev/ttyUSB0"
-            ],
+            "program": "${file}",
             "cwd": "${workspaceRoot}",
             "env": {},
             "envFile": "${workspaceRoot}/.env",

Firmware/MotorControl/commands.cpp

@@ -198,9 +198,12 @@ constexpr size_t NUM_ENDPOINTS = sizeof(endpoints) / sizeof(endpoints[0]);
 // breaks our packet boundaries. For now we just neglect this. If you happen to
 // be limited by such a platform, you should reconsider your life choices
 // or as a workaround enable this:
+
+//Oskar: Put switches like this at top of file
 //#define STREAM_ON_USB
 
 
+
 #ifdef STREAM_ON_USB
 class USBSender : public StreamSink {
 public:
@@ -212,6 +215,8 @@ public:
             while (CDC_Transmit_FS(
                     const_cast<uint8_t*>(buffer) /* casting this const away is safe because...
                     well... it's not actually. Stupid STM. */, chunk) != USBD_OK)
+                //Oskar: we made a semaphore sem_usb_tx that guards the USB tx resource,
+                // that you can wait for to see if busy. Check _write in syscalls.c on devel for example use
                 osDelay(1);
             buffer += chunk;
             length -= chunk;
@@ -237,6 +242,8 @@ public:
         while (CDC_Transmit_FS(
             const_cast<uint8_t*>(buffer) /* casting this const away is safe because...
             well... it's not actually. Stupid STM. */, length) != USBD_OK)
+            //Oskar: we made a semaphore sem_usb_tx that guards the USB tx resource,
+            // that you can wait for to see if busy. Check _write in syscalls.c on devel for example use
             osDelay(1);
         return 0;
     }
@@ -254,6 +261,8 @@ public:
         // Loop until the UART is ready
         // TODO: implement ring buffer to get a more continuous stream of data
         while (huart4.gState != HAL_UART_STATE_READY)
+            //Oskar: we made a semaphore sem_uart_dma that guards the UART tx resource,
+            // that you can wait for to see if busy. Check _write in syscalls.c on devel for example use
             osDelay(1);
         // memcpy data into uart_tx_buf
         memcpy(tx_buf_, buffer, length);
@@ -318,12 +327,16 @@ void communication_task(void const * argument) {
             uint8_t c = dma_circ_buffer[last_rcv_idx];
             if (++last_rcv_idx == UART_RX_BUFFER_SIZE)
                 last_rcv_idx = 0;
+            //Oskar: we don't have to process 1 byte at a time,
+            // we can process up to MIN(last_rcv_idx, UART_RX_BUFFER_SIZE-1)
             UART4_stream_sink.process_bytes(&c, 1);
         }
 
         // When we reach here, we are out of immediate characters to fetch out of UART buffer
         // Now we check if there is any USB processing to do: we wait for up to 1 ms,
         // before going back to checking UART again.
+
+        //Oskar: Beware of changes in devel here when merging.
         int USB_check_timeout = 1;
         int32_t status = osSemaphoreWait(sem_usb_irq, USB_check_timeout);
         if (status == osOK) {
@@ -338,6 +351,11 @@ void communication_task(void const * argument) {
     vTaskDelete(osThreadGetId());
 }
 
+//Oskar: can you also do a ENABLE_LEGACY_PROTOCOL case for UART?
+// If this has to be exclusive of the new protocol, that's fine: it 
+// lets us move on and upgrade the arduino library later.
+// Please test that it still works on an arduino.
+
 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

tools/demo.py

@@ -8,7 +8,10 @@ import time
 import math
 
 # Find a connected ODrive (this will block until you connect one)
-my_drive = odrive.core.find_any(printer=print)
+odrives = odrive.core.find_all(printer=print)
+odrives = list(odrives) #force eval of generator to test finding functions
+my_drive = odrives[0]
+# my_drive = odrive.core.find_any(printer=print)
 
 # The above call returns a python object with a dynamically generated type. The
 # type hierarchy will correspond to the endpoint list in `MotorControl/protocol.cpp`.
@@ -29,7 +32,7 @@ my_drive.motor0.set_pos_setpoint(0.0, 0.0, 0.0)
 
 # little sine wave to test
 t0 = time.monotonic()
-while True:
+while False:
     setpoint = 10000.0 * math.sin((time.monotonic() - t0)*2)
     print("goto " + str(int(setpoint)))
     my_drive.motor0.set_pos_setpoint(setpoint, 0.0, 0.0)
@@ -39,7 +42,7 @@ while True:
 # Some more things you can try:
 
 # Write to a read-only property:
-# my_drive.vbus_voltage = 5  # fails with `AttributeError: can't set attribute`
+my_drive.vbus_voltage = 11.0  # fails with `AttributeError: can't set attribute`
 
 # Assign an incompatible value:
 # my_drive.motor0.pos_setpoint = "I like trains"  # fails with `TypeError: expected value of type float`

tools/odrive/core.py

@@ -68,12 +68,16 @@ def call_remote_function(channel, trigger_id, arg_properties, *args):
         arg_properties[i].fset(None, args[i])
     channel.remote_endpoint_operation(trigger_id, None, True, 0)
 
+#Oskar: setattr_or_raise_if_undefined
 def raise_if_undefined(self, name, value):
     """
     If employed as an object's __setattr__ function, this function
     makes sure that an assignment to an undefined attribute doesn't
     create a new attribute but instead raises an exception
     """
+    #Oskar: hasattr internally calls fget to determine if the attribute exists,
+    # which unnessecarily creates bus traffic. We should try to solve this.
+    # Step-in on the hasattr line in the debugger to see this.
     if hasattr(self, name):
         object.__setattr__(self, name, value)
     else:
@@ -124,6 +128,8 @@ def create_property(name, json_data, channel, printer):
         printer("property {} has no specified ID".format(name))
         return None
 
+    #Oskar: Bug: json_data calls this "access", but we look for "mode".
+    # The default should probably be "r" anyway, it's safer I'd say.
     access_mode = json_data.get("mode", "rw")
     return SimpleDeviceProperty(channel, id_str, property_type,
                                 struct_format,
@@ -250,20 +256,30 @@ def find_usb_channels(vid_pid_pairs=odrive.util.USB_VID_PID_PAIRS, printer=nopri
                     continue
                 raise
 
+def find_dev_serial_ports(search_regex):
+    try:
+        return ['/dev/' + x for x in filter(re.compile(search_regex).search, os.listdir('/dev'))]
+    except FileNotFoundError:
+        return []
+
 def find_serial_channels(printer=noprint):
     """
     Scans for serial ports.
     Returns a generator of odrive.protocol.Channel objects.
     Not every returned object necessarily represents a compatible device.
     """
+
+    #Oskar: Why not just use this tool to find the available ports?
+    # https://pyserial.readthedocs.io/en/latest/tools.html#module-serial.tools.list_ports
+
     # Real serial ports or USB-Serial converters
-    linux_real_serial_ports = ['/dev/' + x for x in filter(re.compile(r'^ttyUSB').search, os.listdir('/dev'))]
+    linux_real_serial_ports = find_dev_serial_ports(r'^ttyUSB')
     windows_real_serial_ports = [ "COM1", "COM2", "COM3", "COM4" ]
 
     # Serial devices that are exposed by the platform
     # for the device's USB connection
-    linux_usb_serial_ports = ['/dev/' + x for x in filter(re.compile(r'^ttyACM').search, os.listdir('/dev'))]
-    macos_usb_serial_ports = ['/dev/' + x for x in filter(re.compile(r'^tty\.usbmodem').search, os.listdir('/dev'))]
+    linux_usb_serial_ports = find_dev_serial_ports(r'^ttyACM')
+    macos_usb_serial_ports = find_dev_serial_ports(r'^tty\.usbmodem')
 
     for port in linux_real_serial_ports + windows_real_serial_ports + linux_usb_serial_ports + macos_usb_serial_ports:
         try:

tools/odrive/protocol.py

@@ -56,6 +56,10 @@ class ChannelBrokenException(Exception):
 class DeviceInitException(Exception):
     pass
 
+#Oskar: I would just get rid of these "abstract classes",
+# I think just looking and seeing that the classes have
+# a process_packet or get_packet is enough.
+
 class StreamSource(object):
     pass
 
@@ -171,6 +175,7 @@ class Channel(PacketSink):
     _expected_acks = {}
 
     # Chose these parameters to be sensible for a specific transport layer
+    #Oskar: it's a timeout, not delay.
     _resend_delay = 5.0     # [s]
     _send_attempts = 5
 
@@ -203,11 +208,11 @@ class Channel(PacketSink):
 
         crc16 = calc_crc16(CRC16_INIT, packet)
         if (endpoint_id & 0x7fff == 0):
-            footer = PROTOCOL_VERSION
+            trailer = PROTOCOL_VERSION
         else:
-            footer = self._interface_definition_crc
-        #print("append footer " + footer)
-        packet = packet + struct.pack('<H', footer)
+            trailer = self._interface_definition_crc
+        #print("append trailer " + trailer)
+        packet = packet + struct.pack('<H', trailer)
 
         if (expect_ack):
             self._expected_acks[seq_no] = None

tools/test_communication.py

@@ -55,6 +55,9 @@ def print_usage():
 # that will leave that odrive variable in scope for the interactive session; just give the user some instructions
 # that they can then do stuff like odrive.[tabcomplete]
 
+# We can also make a function odrive.send_legacy_cmd(cmd_str), which is important for some features that
+# we haven't ported yet.
+
 def command_prompt_loop(device, history):
   """
   Presents the command prompt indefinitely until something goes wrong