Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Roderick Colenbrander <roderick@gaikai.com>]

Hi Barnabás,

Thanks for all your feedback (including other patches).

On Thu, Jan 7, 2021 at 10:52 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
>
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > The DualSense features an accelerometer and gyroscope. The data is
> > embedded into the main HID input reports. Expose both sensors through
> > through a separate evdev node.
> >
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > [...]
> > +static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
> > +             int gyro_range, int gyro_res)
> > +{
> > +     struct input_dev *sensors;
> > +     int ret;
> > +
> > +     sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
> > +     if (IS_ERR(sensors))
> > +             return ERR_PTR(-ENOMEM);
>
> I know that at the moment ENOMEM is the only possible error, but I believe
> `return ERR_CAST(sensors);` would be better. (Or even just `return sensors;`.)
>
>
> > +
> > +     __set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> > +
> > +     /* Accelerometer */
> > +     input_set_abs_params(sensors, ABS_X, -accel_range, accel_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_Y, -accel_range, accel_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_Z, -accel_range, accel_range, 16, 0);
> > +     input_abs_set_res(sensors, ABS_X, accel_res);
> > +     input_abs_set_res(sensors, ABS_Y, accel_res);
> > +     input_abs_set_res(sensors, ABS_Z, accel_res);
> > +
> > +     /* Gyroscope */
> > +     input_set_abs_params(sensors, ABS_RX, -gyro_range, gyro_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_RY, -gyro_range, gyro_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_RZ, -gyro_range, gyro_range, 16, 0);
> > +     input_abs_set_res(sensors, ABS_RX, gyro_res);
> > +     input_abs_set_res(sensors, ABS_RY, gyro_res);
> > +     input_abs_set_res(sensors, ABS_RZ, gyro_res);
> > +
> > +     ret = input_register_device(sensors);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     return sensors;
> > +}
> > [...]
> > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > +{
> > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > +     short gyro_speed_plus, gyro_speed_minus;
> > +     short acc_x_plus, acc_x_minus;
> > +     short acc_y_plus, acc_y_minus;
> > +     short acc_z_plus, acc_z_minus;
> > +     int speed_2x;
> > +     int range_2g;
> > +     int ret = 0;
> > +     uint8_t *buf;
> > +
> > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > +     if (!buf)
> > +             return -ENOMEM;
> > +
> > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> I think it would be better if lines were aligned. I have missed this in other patches,
> so if you decide to make this change, please do it everywhere.

What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?

I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.

>
> > +     if (ret < 0)
> > +             goto err_free;
> > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
>
> As per coding style[1], please either use {} for all branches, or just drop the
> `else` and maybe add a new line:
>
> ```
> if (ret < 0)
>   goto ...
>
> if (ret != ...) {
>   ...
> }
> ```
>
>
> > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
>
> I think this message could be improved to better pinpoint the exact problem
> that triggered it.
>
>
> > +             ret = -EINVAL;
> > +             goto err_free;
> > +     }
> > +
> > +     gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
> > +     gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
> > +     gyro_roll_bias   = get_unaligned_le16(&buf[5]);
> > +     gyro_pitch_plus  = get_unaligned_le16(&buf[7]);
> > +     gyro_pitch_minus = get_unaligned_le16(&buf[9]);
> > +     gyro_yaw_plus    = get_unaligned_le16(&buf[11]);
> > +     gyro_yaw_minus   = get_unaligned_le16(&buf[13]);
> > +     gyro_roll_plus   = get_unaligned_le16(&buf[15]);
> > +     gyro_roll_minus  = get_unaligned_le16(&buf[17]);
> > +     gyro_speed_plus  = get_unaligned_le16(&buf[19]);
> > +     gyro_speed_minus = get_unaligned_le16(&buf[21]);
> > +     acc_x_plus       = get_unaligned_le16(&buf[23]);
> > +     acc_x_minus      = get_unaligned_le16(&buf[25]);
> > +     acc_y_plus       = get_unaligned_le16(&buf[27]);
> > +     acc_y_minus      = get_unaligned_le16(&buf[29]);
> > +     acc_z_plus       = get_unaligned_le16(&buf[31]);
> > +     acc_z_minus      = get_unaligned_le16(&buf[33]);
> > +
> > +     /* Set gyroscope calibration and normalization parameters.
> > +      * Data values will be normalized to 1/DS_GYRO_RES_PER_DEG_S degree/s.
> > +      */
>
> A small note, as written in [2], the preferred style of multi-line comments is different,
> so you might want to change the comments. If you decide to make this change, please
> do it everywhere.
>
>
> > +     speed_2x = (gyro_speed_plus + gyro_speed_minus);
> > +     ds->gyro_calib_data[0].abs_code = ABS_RX;
> > +     ds->gyro_calib_data[0].bias = gyro_pitch_bias;
> > +     ds->gyro_calib_data[0].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> > +     ds->gyro_calib_data[0].sens_denom = gyro_pitch_plus - gyro_pitch_minus;
> > +
> > +     ds->gyro_calib_data[1].abs_code = ABS_RY;
> > +     ds->gyro_calib_data[1].bias = gyro_yaw_bias;
> > +     ds->gyro_calib_data[1].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> > +     ds->gyro_calib_data[1].sens_denom = gyro_yaw_plus - gyro_yaw_minus;
> > +
> > +     ds->gyro_calib_data[2].abs_code = ABS_RZ;
> > +     ds->gyro_calib_data[2].bias = gyro_roll_bias;
> > +     ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> > +     ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
> > +
> > +     /* Set accelerometer calibration and normalization parameters.
> > +      * Data values will be normalized to 1/DS_ACC_RES_PER_G G.
>                                                                 ^
> Minor thing, but I believe it should be 'g', not 'G'?
>
>
> > +      */
> > +     range_2g = acc_x_plus - acc_x_minus;
> > +     ds->accel_calib_data[0].abs_code = ABS_X;
> > +     ds->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
> > +     ds->accel_calib_data[0].sens_numer = 2*DS_ACC_RES_PER_G;
> > +     ds->accel_calib_data[0].sens_denom = range_2g;
> > +
> > +     range_2g = acc_y_plus - acc_y_minus;
> > +     ds->accel_calib_data[1].abs_code = ABS_Y;
> > +     ds->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
> > +     ds->accel_calib_data[1].sens_numer = 2*DS_ACC_RES_PER_G;
> > +     ds->accel_calib_data[1].sens_denom = range_2g;
> > +
> > +     range_2g = acc_z_plus - acc_z_minus;
> > +     ds->accel_calib_data[2].abs_code = ABS_Z;
> > +     ds->accel_calib_data[2].bias = acc_z_plus - range_2g / 2;
> > +     ds->accel_calib_data[2].sens_numer = 2*DS_ACC_RES_PER_G;
> > +     ds->accel_calib_data[2].sens_denom = range_2g;
> > +
> > +err_free:
> > +     kfree(buf);
> > +     return ret;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -319,6 +469,7 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       struct dualsense_input_report *ds_report;
> >       uint8_t battery_data, battery_capacity, charging_status, value;
> >       int battery_status;
> > +     uint16_t sensor_timestamp;
> >       unsigned long flags;
> >       int i;
> >
> > @@ -361,6 +512,44 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> >       input_sync(ds->gamepad);
> >
> > +     /* Parse and calibrate gyroscope data. */
> > +     for (i = 0; i < 3; i++) {
>
> I think `i < ARRAY_SIZE(...)` would be better.
> And I would add a `static_assert(ARRAY_SIZE(ds_report->gyro) == ARRAY_SIZE(ds->gyro_calib_data))`
> somewhere around here just to be safe. Or define a new constant like `DS_GYRO_DIMS`
> and use that to define the arrays. Or both. *
>
>
> > +             int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
> > +             int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
> > +                             raw_data - ds->gyro_calib_data[i].bias,
> > +                             ds->gyro_calib_data[i].sens_denom);
>
> I believe it would be better if the second and third lines was aligned. **
>
>
> > +
> > +             input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
> > +     }
> > +
> > +     /* Parse and calibrate accelerometer data. */
> > +     for (i = 0; i < 3; i++) {
>
> Same here. *
>
>
> > +             int raw_data = (short)le16_to_cpu(ds_report->accel[i]);
> > +             int calib_data = mult_frac(ds->accel_calib_data[i].sens_numer,
> > +                             raw_data - ds->accel_calib_data[i].bias,
> > +                             ds->accel_calib_data[i].sens_denom);
>
> Same here. **
>
>
> > +
> > +             input_report_abs(ds->sensors, ds->accel_calib_data[i].abs_code, calib_data);
> > +     }
> > +
> > +     /* Convert timestamp (in 0.33us unit) to timestamp_us */
> > +     sensor_timestamp = le32_to_cpu(ds_report->sensor_timestamp);
> > +     if (!ds->sensor_timestamp_initialized) {
> > +             ds->sensor_timestamp_us = sensor_timestamp / 3;
> > +             ds->sensor_timestamp_initialized = true;
> > +     } else {
> > +             uint32_t delta;
> > +
> > +             if (ds->prev_sensor_timestamp > sensor_timestamp)
> > +                     delta = (U32_MAX - ds->prev_sensor_timestamp + sensor_timestamp + 1);
> > +             else
> > +                     delta = sensor_timestamp - ds->prev_sensor_timestamp;
> > +             ds->sensor_timestamp_us += delta / 3;
> > +     }
> > +     ds->prev_sensor_timestamp = sensor_timestamp;
> > +     input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
> > +     input_sync(ds->sensors);
> > +
> >       for (i = 0; i < 2; i++) {
> >               bool active = (ds_report->points[i].contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
> >
> > @@ -446,12 +635,25 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       }
> >       snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
> >
> > +     ret = dualsense_get_calibration_data(ds);
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to get calibration data from DualSense\n");
> > +             goto err;
> > +     }
> > +
> >       ds->gamepad = ps_gamepad_create(hdev);
> >       if (IS_ERR(ds->gamepad)) {
> >               ret = PTR_ERR(ds->gamepad);
> >               goto err;
> >       }
> >
> > +     ds->sensors = ps_sensors_create(hdev, DS_ACC_RANGE, DS_ACC_RES_PER_G,
> > +                     DS_GYRO_RANGE, DS_GYRO_RES_PER_DEG_S);
>
> I believe it would be better if the second line was aligned to the `h` in 'hdev'.
>
>
> > +     if (IS_ERR(ds->sensors)) {
> > +             ret = PTR_ERR(ds->sensors);
> > +             goto err;
> > +     }
> > +
> >       ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> >       if (IS_ERR(ds->touchpad)) {
> >               ret = PTR_ERR(ds->touchpad);
> >
>
>
> [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
> [2]: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
>
>
> Regards,
> Barnabás Pőcze


Regards,
Roderick Colenbrander