Add a bunch of proto documentation

firmware/src/proto_gen/smartknob.pb.h

@@ -10,6 +10,7 @@
 #endif
 
 /* Struct definitions */
+/* * Lets the host know that a ToSmartknob message was received and should not be retried. */
 typedef struct _PB_Ack {
     uint32_t nonce;
 } PB_Ack;
@@ -19,7 +20,26 @@ typedef struct _PB_Log {
 } PB_Log;
 
 typedef struct _PB_SmartKnobConfig {
+    /* *
+ Set the integer position.
+
+ Note: in order to make SmartKnobConfig apply idempotently, the current position
+ will only be set to this value when it changes compared to a previous config (and
+ NOT compared to the current state!). So by default, if you send a config position
+ of 5 and the current position is 3, the position may remain at 3 if the config
+ change to 5 was previously handled. If you need to force a position update, see
+ position_nonce. */
     int32_t position;
+    /* *
+ Set the fractional position. Typical range: (-snap_point, snap_point).
+
+ Actual range is technically unbounded, but in practice this value will be compared
+ against snap_point on the next control loop, so any value beyond the snap_point will
+ generally result in an integer position change (unless position is already at a
+ limit).
+
+ Note: idempotency implications noted in the documentation for `position` apply here
+ as well */
     float sub_position_unit;
     /* *
  Position is normally only applied when it changes, but sometimes it's desirable
@@ -28,15 +48,69 @@ typedef struct _PB_SmartKnobConfig {
 
  NOTE: Must be < 256 */
     uint8_t position_nonce;
+    /* * Minimum position allowed. */
     int32_t min_position;
+    /* *
+ Maximum position allowed.
+
+ If this is the same as min_position, there will only be one allowed position.
+
+ If this is less than min_position, bounds will be disabled. */
     int32_t max_position;
+    /* * The angular "width" of each position/detent, in radians. */
     float position_width_radians;
+    /* *
+ Strength of detents to apply. Typical range: [0, 1].
+
+ A value of 0 disables detents.
+
+ Values greater than 1 are not recommended and may lead to unstable behavior. */
     float detent_strength_unit;
+    /* *
+ Strength of endstop torque to apply at min/max bounds. Typical range: [0, 1].
+
+ A value of 0 disables endstop torque, but does not make position unbounded, meaning
+ the knob will not try to return to the valid region. For unbounded rotation, use
+ min_position and max_position.
+
+ Values greater than 1 are not recommended and may lead to unstable behavior. */
     float endstop_strength_unit;
+    /* *
+ Fractional (sub-position) threshold where the position will increment/decrement.
+ Typical range: (0.5, 1.5).
+
+ This defines how hysteresis is applied to positions, which is why values > */
     float snap_point;
+    /* *
+ Arbitrary 50-byte string representing this "config". This can be used to identify major
+ config/mode changes. The value will be echoed back to the host via a future State's
+ embedded config field so the host can use this value to determine the mode that was
+ in effect at the time of the State snapshot instead of having to infer it from the
+ other config fields. */
     char text[51];
+    /* *
+ For a "magnetic" detent mode - where not all positions should have detents - this
+ specifies which positions (up to 5) have detents enabled. The knob will feel like it
+ is "magnetically" attracted to those positions, and will rotate smoothy past all
+ other positions.
+
+ If you want to have more than 5 magnetic detent positions, you will need to dynamically
+ update this list as the knob is rotated. A recommended approach is to always send the
+ _nearest_ 5 detent positions, and send a new Config message whenever the list of
+ positions nearest the current position (as reported via State messages) changes.
+
+ This approach enables effectively unbounded detent positions while keeping Config
+ bounded in size, and is resilient against tightly-packed detents with fast rotation
+ since multiple detent positions can be sent in advance; a full round-trip Config-State
+ isn't needed between each detent in order to keep up. */
     pb_size_t detent_positions_count;
     int32_t detent_positions[5];
+    /* *
+ Advanced feature for shifting the defined snap_point away from the center (position 0)
+ for implementing asymmetric detents. Typical value: 0 (symmetric detent force).
+
+ This can be used to create detents that will hold the position when carefully released,
+ but can be easily disturbed to return "home" towards position 0. */
     float snap_point_bias;
     /* *
  Hue (0-255) for all 8 ring LEDs, if supported. Note: this will likely be replaced
@@ -45,11 +119,37 @@ typedef struct _PB_SmartKnobConfig {
 } PB_SmartKnobConfig;
 
 typedef struct _PB_SmartKnobState {
+    /* * Current integer position of the knob. (Detent resolution is at integer positions) */
     int32_t current_position;
+    /* *
+ Current fractional position. Typically will only range from (-snap_point, snap_point)
+ since further rotation will result in the integer position changing, but may exceed
+ those values if snap_point_bias is non-zero, or if the knob is at a bound. When the
+ knob is at a bound, this value can grow endlessly as the knob is rotated further past
+ the bound.
+
+ When visualizing sub_position_unit, you will likely want to apply a rubber-band easing
+ function past the bounds; a sublinear relationship will help suggest that a bound has
+ been reached. */
     float sub_position_unit;
+    /* *
+ Current SmartKnobConfig in effect at the time of this State snapshot.
+
+ Beware that this config contains position and sub_position_unit values, not to be
+ confused with the top level current_position and sub_position_unit values in this State
+ message. The position values in the embedded config message will almost never be useful
+ to you; you probably want to be reading the top level values from the State message. */
     bool has_config;
     PB_SmartKnobConfig config;
-    /* * Value that changes each time the knob is pressed */
+    /* *
+ Value that changes each time the knob is pressed. Does not change when a press is released.
+
+ Why this press state a "nonce" rather than a simple boolean representing the current
+ "pressed" state? It makes the protocol more robust to dropped/lost State messages; if
+ the knob was pressed/released quickly and State messages happened to be dropped during
+ that time, the press would be completely lost. Using a nonce allows the host to recognize
+ that a press has taken place at some point even if the State was lost during the press
+ itself. Is this overkill? Probably, let's revisit in future protocol versions. */
     uint8_t press_nonce;
 } PB_SmartKnobState;
 

proto/smartknob.proto

@@ -29,6 +29,7 @@ message FromSmartKnob {
     }
 }
 
+/** Lets the host know that a ToSmartknob message was received and should not be retried. */
 message Ack {
     uint32 nonce = 1;
 }
@@ -38,17 +39,70 @@ message Log {
 }
 
 message SmartKnobState {
+    /** Current integer position of the knob. (Detent resolution is at integer positions) */
     int32 current_position = 1;
+
+    /**
+     * Current fractional position. Typically will only range from (-snap_point, snap_point)
+     * since further rotation will result in the integer position changing, but may exceed
+     * those values if snap_point_bias is non-zero, or if the knob is at a bound. When the
+     * knob is at a bound, this value can grow endlessly as the knob is rotated further past
+     * the bound.
+     *
+     * When visualizing sub_position_unit, you will likely want to apply a rubber-band easing
+     * function past the bounds; a sublinear relationship will help suggest that a bound has
+     * been reached.
+     */
     float sub_position_unit = 2;
+
+    /**
+     * Current SmartKnobConfig in effect at the time of this State snapshot.
+     *
+     * Beware that this config contains position and sub_position_unit values, not to be
+     * confused with the top level current_position and sub_position_unit values in this State
+     * message. The position values in the embedded config message will almost never be useful
+     * to you; you probably want to be reading the top level values from the State message.
+     */
     SmartKnobConfig config = 3;
 
-    /** Value that changes each time the knob is pressed */
+    /**
+     * Value that changes each time the knob is pressed. Does not change when a press is released.
+     *
+     * Why this press state a "nonce" rather than a simple boolean representing the current
+     * "pressed" state? It makes the protocol more robust to dropped/lost State messages; if
+     * the knob was pressed/released quickly and State messages happened to be dropped during
+     * that time, the press would be completely lost. Using a nonce allows the host to recognize
+     * that a press has taken place at some point even if the State was lost during the press
+     * itself. Is this overkill? Probably, let's revisit in future protocol versions.
+     */
     uint32 press_nonce = 4 [(nanopb).int_size = IS_8];
 }
 
 
 message SmartKnobConfig {
+    /**
+     * Set the integer position.
+     *
+     * Note: in order to make SmartKnobConfig apply idempotently, the current position
+     * will only be set to this value when it changes compared to a previous config (and
+     * NOT compared to the current state!). So by default, if you send a config position
+     * of 5 and the current position is 3, the position may remain at 3 if the config
+     * change to 5 was previously handled. If you need to force a position update, see
+     * position_nonce.
+     */
     int32 position = 1;
+
+    /**
+     * Set the fractional position. Typical range: (-snap_point, snap_point).
+     *
+     * Actual range is technically unbounded, but in practice this value will be compared
+     * against snap_point on the next control loop, so any value beyond the snap_point will
+     * generally result in an integer position change (unless position is already at a
+     * limit).
+     *
+     * Note: idempotency implications noted in the documentation for `position` apply here
+     * as well
+     */
     float sub_position_unit = 2;
     
     /**
@@ -60,14 +114,83 @@ message SmartKnobConfig {
      */
     uint32 position_nonce = 3 [(nanopb).int_size = IS_8];
 
+    /** Minimum position allowed. */
     int32 min_position = 4;
+
+    /**
+     * Maximum position allowed.
+     *
+     * If this is the same as min_position, there will only be one allowed position.
+     *
+     * If this is less than min_position, bounds will be disabled.
+     */
     int32 max_position = 5;
+
+    /** The angular "width" of each position/detent, in radians. */
     float position_width_radians = 6;
+
+    /**
+     * Strength of detents to apply. Typical range: [0, 1].
+     *
+     * A value of 0 disables detents.
+     *
+     * Values greater than 1 are not recommended and may lead to unstable behavior.
+     */
     float detent_strength_unit = 7;
+
+    /**
+     * Strength of endstop torque to apply at min/max bounds. Typical range: [0, 1].
+     *
+     * A value of 0 disables endstop torque, but does not make position unbounded, meaning
+     * the knob will not try to return to the valid region. For unbounded rotation, use
+     * min_position and max_position.
+     *
+     * Values greater than 1 are not recommended and may lead to unstable behavior.
+     */
     float endstop_strength_unit = 8;
+
+    /**
+     * Fractional (sub-position) threshold where the position will increment/decrement.
+     * Typical range: (0.5, 1.5).
+     *
+     * This defines how hysteresis is applied to positions, which is why values >
+     */
     float snap_point = 9;
+
+    /**
+     * Arbitrary 50-byte string representing this "config". This can be used to identify major
+     * config/mode changes. The value will be echoed back to the host via a future State's
+     * embedded config field so the host can use this value to determine the mode that was
+     * in effect at the time of the State snapshot instead of having to infer it from the
+     * other config fields.
+     */
     string text = 10 [(nanopb).max_length = 50];
+
+    /**
+     * For a "magnetic" detent mode - where not all positions should have detents - this
+     * specifies which positions (up to 5) have detents enabled. The knob will feel like it
+     * is "magnetically" attracted to those positions, and will rotate smoothy past all
+     * other positions.
+     *
+     * If you want to have more than 5 magnetic detent positions, you will need to dynamically
+     * update this list as the knob is rotated. A recommended approach is to always send the
+     * _nearest_ 5 detent positions, and send a new Config message whenever the list of
+     * positions nearest the current position (as reported via State messages) changes.
+     *
+     * This approach enables effectively unbounded detent positions while keeping Config
+     * bounded in size, and is resilient against tightly-packed detents with fast rotation
+     * since multiple detent positions can be sent in advance; a full round-trip Config-State
+     * isn't needed between each detent in order to keep up.
+     */
     repeated int32 detent_positions = 11 [(nanopb).max_count = 5];
+
+    /**
+     * Advanced feature for shifting the defined snap_point away from the center (position 0)
+     * for implementing asymmetric detents. Typical value: 0 (symmetric detent force).
+     *
+     * This can be used to create detents that will hold the position when carefully released,
+     * but can be easily disturbed to return "home" towards position 0.
+     */
     float snap_point_bias = 12;
 
     /**