On 18/12/2023 17:39, Rahul Rameshbabu wrote: > [...] > > Hi Guilherme, > > I agree with the previous hunks you added and can see how those could > trigger the divide-by-zero issue you were seeing. However, I am not sure > if this hunk that I have left makes sense. > > Reason being, is that the hid-nintendo driver has a special conditional > to prevent divide-by-zero in this case without this change. > > 1. If the first packet has not been received by the IMU, set > imu_avg_delta_ms to JC_IMU_DFLT_AVG_DELTA_MS. > 2. Only change imu_avg_delta_ms when imu_delta_samples_count >= > JC_IMU_SAMPLES_PER_DELTA_AVG. > 3. If that change leads to imu_avg_delta_ms being 0, set it to 1. > > With this logic as-is, I do not see how a divide by zero could have > occurred in this specific path without your change. I might be missing > something, but I wanted to make sure to avoid integrating a hunk that > does not help. > > Would it be possible to retest without this hunk? > Hi Rahul, thanks for your review. I think I see ... I covered both bases in this patch, but IIUC after your points above and better looking the code: (a) imu_avg_delta_ms is set to JC_IMU_DFLT_AVG_DELTA_MS and it can only change iff imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG. (b) But the if path related with the imu_delta_samples_count is the one that also guards the divide-by-zero, so effectively that error condition cannot happen outside that if path, i.e., my hunk changed nothing. Ugh...my bad. At the same time, the hunk is harmless - it's up to Jiri to decide, we can drop it (either directly by git rebasing or I can send a V2 if Jiri prefers), or we can keep it. I can try to test internally, github testing may be a bit uncertain in the timeframe (specially during holidays season). If Jiri thinks the hunk is harmless and no change is necessary, I'd rather not bother people for testing now (I don't have the HW). Cheers, Guilherme
