| Message ID | 20210102223109.996781-6-roderick@gaikai.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: new driver for PS5 'DualSense' controller | expand |
> +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); > + > + __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; > +} The bits for EV_MSC/MSC_TIMESTAMP events are not set here, hence timestamp events would not delivered: __set_bit(EV_MSC, sensors->evbit); __set_bit(MSC_TIMESTAMP, sensors->mscbit); > 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; This uint16_t variable overflows just after a few events. Since the timestamp from the controller is 32bit and the event value too, I assume this should be too.
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. > + 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
Hi Florian, Thanks for your comments. On Thu, Jan 7, 2021 at 5:34 AM Florian Märkl <linux@florianmaerkl.de> wrote: > > > +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); > > + > > + __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; > > +} > > The bits for EV_MSC/MSC_TIMESTAMP events are not set here, hence > timestamp events would not delivered: > > __set_bit(EV_MSC, sensors->evbit); > __set_bit(MSC_TIMESTAMP, sensors->mscbit); I don't know how I missed this... I remember adding it, but nope apparently not :( > > 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; > > This uint16_t variable overflows just after a few events. Since the > timestamp from the controller is 32bit and the event value too, I assume > this should be too. Agreed. > > -- > Florian Märkl > https://metallic.software Regards, Roderick
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
Hi 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta: > [...] > > > +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? Yes, exactly. > > 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. I think it's a good idea to add such a function sometime. > > > > > > + 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; > > > + } > [...] Regards, Barnabás Pőcze
Hi Roderick, Is there any way to align the sensor timestamps with the real clock on this new device? If so, there's input_set_timestamp api [0] that could be used for setting the timestamps of the actual input_events rather than having to send out parallel MSC_TIMESTAMP messages. It would make it easier for user space to process these events. [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/ On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > Hi > > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta: > > > [...] > > > > +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? > > Yes, exactly. > > > > > > 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. > > I think it's a good idea to add such a function sometime. > > > > > > > > > > > + 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; > > > > + } > > [...] > > > Regards, > Barnabás Pőcze
Hi Siarhei, It might be an idea to indeed use that API. I wasn't aware of its existence. Though I don't fully understand how it works (and how you can guarantee alignment). Unfortunately I don't see any drivers in upstream Linux using it. Do you happen to know of drivers using it? I guess the might be some in Android kernel-common? Thanks, Roderick On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou <svv@google.com> wrote: > > Hi Roderick, > > Is there any way to align the sensor timestamps with the real clock on > this new device? If so, there's input_set_timestamp api [0] that could > be used for setting the timestamps of the actual input_events rather > than having to send out parallel MSC_TIMESTAMP messages. It would make > it easier for user space to process these events. > > [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/ > > > On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > > > Hi > > > > > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta: > > > > > [...] > > > > > +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? > > > > Yes, exactly. > > > > > > > > > > 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. > > > > I think it's a good idea to add such a function sometime. > > > > > > > > > > > > > > > > + 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; > > > > > + } > > > [...] > > > > > > Regards, > > Barnabás Pőcze
This api is used by some of our touch drivers to more accurately set the timestamps of touch events. This allows us to better measure touch latency. An example can be found in [0]. From what I remember, you call this api to apply a specific timestamp to all of the subsequent input_events that are produced. When input_sync happens, this timestamp is erased and you revert to the default behaviour (acquiring a timestamp in evdev) until this api is called again. So if you choose to use this api, you would have to take care to only apply it to the sensor events and not other events (unless you can figure out the timestamps for all), as well as finding a way to align the hardware timestamps with the wall clock. For the touch driver case, it's easy because we are just taking the current time at the interrupt. This still misses the portions where the touch scanning and data preprocessing on the touch IC occurs, but it gets us closer to the real number (for example, it helps account for the i2c/spi data transfer time, which happens after the interrupt). [0] https://github.com/android-linux-stable/bluecross/blob/android-msm-bluecross-4.9/drivers/input/touchscreen/stm/fts.c#L3451 On Fri, Jan 8, 2021 at 9:54 AM Roderick Colenbrander <roderick@gaikai.com> wrote: > > Hi Siarhei, > > It might be an idea to indeed use that API. I wasn't aware of its > existence. Though I don't fully understand how it works (and how you > can guarantee alignment). Unfortunately I don't see any drivers in > upstream Linux using it. Do you happen to know of drivers using it? I > guess the might be some in Android kernel-common? > > Thanks, > Roderick > > On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou <svv@google.com> wrote: > > > > Hi Roderick, > > > > Is there any way to align the sensor timestamps with the real clock on > > this new device? If so, there's input_set_timestamp api [0] that could > > be used for setting the timestamps of the actual input_events rather > > than having to send out parallel MSC_TIMESTAMP messages. It would make > > it easier for user space to process these events. > > > > [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/ > > > > > > On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > > > > > Hi > > > > > > > > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta: > > > > > > > [...] > > > > > > +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? > > > > > > Yes, exactly. > > > > > > > > > > > > > > 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. > > > > > > I think it's a good idea to add such a function sometime. > > > > > > > > > > > > > > > > > > > > > + 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; > > > > > > + } > > > > [...] > > > > > > > > > Regards, > > > Barnabás Pőcze > > > > -- > Roderick Colenbrander > Senior Manager of Hardware & Systems Engineering > Sony Interactive Entertainment LLC > roderick.colenbrander@sony.com
Hi Siarhei,
Thanks for sharing the example. I see now how the API could be used.
In case of DualSense, the 'dualsense_parse_report' call is effectively
the ISR and the timestamp is derived there by the input framework. We
would like to use the hardware timestamp (as in timestamp at which the
device created the event), so we can do accurate motion tracking. In
particular in case of Bluetooth there is variation in timestamps.
We could do our own time tracking by taking an initial CLOCK_MONOTONIC
and then adding our own hardware timestamps to it. Something like:
if (!sensor_timestamp_initialized) {
timestamp = ktime_get();
hw_time0 = get sensor timestamp();
}
hw_delta = get_sensor_timestamp() - hw_time0;
ktime_add_ns(timestamp, hw_delta);
input_set_timestamp(sensor_dev, timestamp);
I just don't know what others would think about such an approach vs
MSC_TIMESTAMP (or we can do both).
Thanks,
Roderick
On Fri, Jan 8, 2021 at 4:11 PM Siarhei Vishniakou <svv@google.com> wrote:
>
> This api is used by some of our touch drivers to more accurately set
> the timestamps of touch events. This allows us to better measure touch
> latency. An example can be found in [0].
>
> From what I remember, you call this api to apply a specific timestamp
> to all of the subsequent input_events that are produced. When
> input_sync happens, this timestamp is erased and you revert to the
> default behaviour (acquiring a timestamp in evdev) until this api is
> called again.
> So if you choose to use this api, you would have to take care to only
> apply it to the sensor events and not other events (unless you can
> figure out the timestamps for all), as well as finding a way to align
> the hardware timestamps with the wall clock.
>
> For the touch driver case, it's easy because we are just taking the
> current time at the interrupt. This still misses the portions where
> the touch scanning and data preprocessing on the touch IC occurs, but
> it gets us closer to the real number (for example, it helps account
> for the i2c/spi data transfer time, which happens after the
> interrupt).
>
>
> [0] https://github.com/android-linux-stable/bluecross/blob/android-msm-bluecross-4.9/drivers/input/touchscreen/stm/fts.c#L3451
>
> On Fri, Jan 8, 2021 at 9:54 AM Roderick Colenbrander
> <roderick@gaikai.com> wrote:
> >
> > Hi Siarhei,
> >
> > It might be an idea to indeed use that API. I wasn't aware of its
> > existence. Though I don't fully understand how it works (and how you
> > can guarantee alignment). Unfortunately I don't see any drivers in
> > upstream Linux using it. Do you happen to know of drivers using it? I
> > guess the might be some in Android kernel-common?
> >
> > Thanks,
> > Roderick
> >
> > On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou <svv@google.com> wrote:
> > >
> > > Hi Roderick,
> > >
> > > Is there any way to align the sensor timestamps with the real clock on
> > > this new device? If so, there's input_set_timestamp api [0] that could
> > > be used for setting the timestamps of the actual input_events rather
> > > than having to send out parallel MSC_TIMESTAMP messages. It would make
> > > it easier for user space to process these events.
> > >
> > > [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
> > >
> > >
> > > On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> > > >
> > > > Hi
> > > >
> > > >
> > > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
> > > >
> > > > > [...]
> > > > > > > +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?
> > > >
> > > > Yes, exactly.
> > > >
> > > >
> > > > >
> > > > > 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.
> > > >
> > > > I think it's a good idea to add such a function sometime.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > + 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;
> > > > > > > + }
> > > > > [...]
> > > >
> > > >
> > > > Regards,
> > > > Barnabás Pőcze
> >
> >
> >
> > --
> > Roderick Colenbrander
> > Senior Manager of Hardware & Systems Engineering
> > Sony Interactive Entertainment LLC
> > roderick.colenbrander@sony.com
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c index 06e946c45c64..80b0dcdbfe41 100644 --- a/drivers/hid/hid-playstation.c +++ b/drivers/hid/hid-playstation.c @@ -33,8 +33,18 @@ struct ps_device { int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size); }; +/* Calibration data for playstation motion sensors. */ +struct ps_calibration_data { + int abs_code; + short bias; + int sens_numer; + int sens_denom; +}; + #define DS_INPUT_REPORT_USB 0x01 +#define DS_FEATURE_REPORT_CALIBRATION 5 +#define DS_FEATURE_REPORT_CALIBRATION_SIZE 41 #define DS_FEATURE_REPORT_PAIRING_INFO 9 #define DS_FEATURE_REPORT_PAIRING_INFO_SIZE 19 @@ -67,13 +77,27 @@ struct ps_device { #define DS_TOUCH_POINT_INACTIVE BIT(7) /* DualSense hardware limits */ +#define DS_ACC_RES_PER_G 8192 +#define DS_ACC_RANGE (4*DS_ACC_RES_PER_G) +#define DS_GYRO_RES_PER_DEG_S 1024 +#define DS_GYRO_RANGE (2048*DS_GYRO_RES_PER_DEG_S) #define DS_TOUCHPAD_WIDTH 1920 #define DS_TOUCHPAD_HEIGHT 1080 struct dualsense { struct ps_device base; struct input_dev *gamepad; + struct input_dev *sensors; struct input_dev *touchpad; + + /* Calibration data for accelerometer and gyroscope. */ + struct ps_calibration_data accel_calib_data[3]; + struct ps_calibration_data gyro_calib_data[3]; + + /* Timestamp for sensor data */ + bool sensor_timestamp_initialized; + uint32_t prev_sensor_timestamp; + uint32_t sensor_timestamp_us; }; struct dualsense_touch_point { @@ -255,6 +279,41 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev) return gamepad; } +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); + + __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 struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height, unsigned int num_contacts) { @@ -283,6 +342,97 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, return touchpad; } +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); + if (ret < 0) + goto err_free; + else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) { + hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n"); + 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. + */ + 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. + */ + 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++) { + 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); + + 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++) { + 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); + + 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); + 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);