Hi Grant, > > Some comments on your coding style now, although the patch is quite > > hard to read (not sure there's anything you can do about this > > though). > > I tried with and without 'p' diff option, would changing the number > of context lines stop diff from 'merging' a replacement function? > No, it didn't -- would appreciate a cluebat on getting a better > diff? No idea, unfortunately. I've dreamt of a "--do-not-try-to-be-clever" option to GNU diff, but there isn't. Nevermind, in this case you can look at the new code directly rather than the diff. > > I understand that new_min++ is for rounding purposes, but it doesn't > > sound good to me, because of cumulated errors. > > > > Consider the case where the user asked for a low limit of 2630. > > new_min will be computed as 513, new_div as 0. First iteration of the > > loop, new_div becomes 1, new_min becomes 257. Second iteration of the > > loop, new_div becomes 2, new_min becomes 129. We're happy and leave > > the loop. The effective min limit is now (1350000 + (2 * 129)) / (4 * > > 129) = 2616 RPM. > > > > Without the rounding now. First iteration of the loop, new_div > > becomes 1, new_min becomes 256. Second iteration of the loop, new_div > > becomes 2, new_min becomes 128. We're happy and leave the loop. The > > effective min limit is now (1350000 + (2 * 128)) / (4 * 128) = 2637 > > RPM, which is nearer from 2630 than 2616 was. > > And the other side of the same decision point: 255? What do you mean exactly? If that was the sense of your question, I agree that in some cases your rounding will happen to do the right thing (e.g. for 2640, your method leads to 2636, mine leads to 2657). My point was that your method would introduce cumulated rounding errors in some cases. I don't think you can argue with that. This is a mathematical fact. So, either do proper rounding, or don't do any rounding at all. Rounding that will work properly for half of the cases and make things worse for the other half isn't worth the additional effort. As the proper rouding looked significantly more expensive, I opted for no rounding at all. If you come with an elegant, proper rounding solution, I'll consider it. Also beware that whatever rounding you do should be taken into consideration in the earlier test which discards the corner cases. Failing to do so might result in overflows. > > And as explained earlier, I don't think you should do that (decrease > > the divider). Increasing is needed, but decreasing isn't. > > Disagree: when using a wide speed range fan user may prefer most > accurate fan speed measurement reading. > > It is the idea that user explicitly selects either fan low speed > limit mode, or wants to monitor fan speed. And this is a bad idea. Asking the user to choose between having a low speed alarm and "better" fan speed measurements is stupid. The whole point of hardware monitoring is to detect hardware problems, and you are almost inviting the user to disable the alarms in exchange of better measurements! Please remember that we are *not* introducing automatic fan clock divider selection to provide stlightly more accurate fan speed readings. We are doing it so that people stop wondering and complaining that their fan speed reads 0 while the fan is spinning. My implementation does just this, while yours fails to address the problem. This is the reason why I can't accept it. Thanks, -- Jean Delvare
