Hi Jean,
On Tue, 2011-03-15 at 12:56 -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 15 Mar 2011 09:23:25 -0700, Guenter Roeck wrote:
> > On Tue, Mar 15, 2011 at 06:27:34AM -0400, Jean Delvare wrote:
> > > The first difference is "hyst" being changed to "crit hyst" for
> > > critical temperature limit hysteresis. The original code enforced every
> > > temperature limit label to 4 characters to keep all lines nicely
> > > aligned, and your change breaks this effort. As the hysteresis value
> > > was always printed right after the limit it related to, on the same
> > > line, the "crit" in front of "hyst" was redundant. With the new code,
> > > there might be a new line in between, which isn't so nice. This should
> > > be addressed in an incremental patch IMHO (I can take care.)
> >
> > ok. Problem though is that there are other sensor strings which don't fit
> > into four characters (crit low for temperatures, more for other sensor types).
> > We would have to change those as well, and possibly specify a fixed length for
> > limit strings associated with other sensor types.
>
> You're right, I've seen this problem too. I'll see if I have ideas how
> to solve it. This is independent from your patch though, so let's get
> your patch applied first.
>
> > > (...)
> > > You want " (" here.
> >
> > Ok. That means the output will be "ALARM (MIN, MAX)".
>
> Correct, as we has before.
>
> > > > (...)
> > > > +static void get_sensor_limit_data(const sensors_chip_name *name,
> > > > + const sensors_feature *feature,
> > > > + const struct sensor_limits *ts,
> > > > + struct sensor_limit_data *ts_data,
> > > > + int *ts_sensors,
> > > > + struct sensor_limit_data *ts_alarm_data,
> > > > + int *ts_alarms)
> > >
> > > The names of the last 4 parameters are really confusing... even worse
> > > than for print_limits()! Please use the same naming convention for both
> > > functions. And in the calling functions, too. And I'm not even sure
> > > what "ts" stands for... "temperature sensor", as a legacy of a time
> > > where this function was only used to print temperature limits?
> >
> > Probably because I started with temperatures.
> >
> > Changed to
> > const struct sensor_subfeatures *sf,
> > struct sensor_subfeature_data *limits,
> > int *num_limits,
> > struct sensor_subfeature_data *alarms,
> > int *num_alarms)
> > which I hope is a bit better.
>
> Yes, much better.
>
> > > (...)
> > > There is something I don't like about this function: you assume that
> > > the provided ts, ts_sensors and ts_alarms are such that all the
> > > subfeatures from ts (including exists and nexists recursions) will fit
> > > in ts_sensors and ts_alarms are. If this assumption fails, you'll
> > > overrun the arrays and corrupt the stack, which is pretty bad. The
> > > recursive relations between the various ts arrays make it easy to get
> > > wrong, methinks.
> > >
> > > Unfortunately I see no way to check the sizes for correctness at build
> > > time, which means we'll have to do it at run time. This could be done
> > > with the current prototype by having the caller pass the array sizes as
> > > the initial values of ts_sensors and ts_alarms. The code would only
> > > have to be adjusted a little. Or maybe you have a better idea?
> > >
> > One other idea would be to dynamically allocate data as needed. Not sure if that
> > would be much better.
>
> I thought about it, but I can't think of an efficient way to do it in
> one pass. And doing it in 2 passes will certainly perform badly.
>
> > For now, I added parameters for max_limits and max_alarms.
>
> Yes, this should work fine.
>
> > > > (...)
> > > > + strcpy(fmt, "%-4s = %+5.1f");
> > > > + strcat(fmt, degstr);
> > >
> > > I strongly discourage the use of strcpy and strcat, especially on stack
> > > buffers, for both safety and performance reasons. I would have
> > > suggested the use of snprintf instead, but...
> >
> > Sure snprintf would be faster ?
>
> Probably not in this case, no, because the strings your manipulate are
> small enough. But with larger strings, repeated strcat are pretty slow.
>
> The main issue if the risk of buffer overflow anyway.
>
> > > > (...)
> > > > +static const struct sensor_limits power_inst_highest[] = {
> > > > + { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, NULL, 0, "max" },
> > > > + { -1, NULL, NULL, 0, NULL }
> > > > +};
> > > > +
> > > > +static const struct sensor_limits power_inst_max[] = {
> > > > + { SENSORS_SUBFEATURE_POWER_MAX, NULL, NULL, 0, "max" },
> > > > + { SENSORS_SUBFEATURE_POWER_CRIT, NULL, NULL, 0, "crit" },
> > > > + { -1, NULL, NULL, 0, NULL }
> > > > +};
> > > > +
> > > > +static const struct sensor_limits power_inst_sensors[] = {
> > > > + { SENSORS_SUBFEATURE_POWER_ALARM, NULL, power_alarms, 1, NULL },
> > > > + { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, power_inst_highest,
> > > > + power_inst_max, 0, "min" },
> > >
> > > This deserves a comment at least... if it is correct at all. Why would
> > > INPUT_HIGHEST require INPUT_LOWEST? I can imagine a chip providing one
> > > without the other (this would even make a lot of sense if you ask me).
> > > I also don't get why MAX and CRIT would be mutually exclusive with
> > > INPUT_LOWEST and INPUT_HIGHEST. Please explain or fix.
> >
> > Backwards compatibility issue. "max" and "min" were traditionally used
> > to display "highest" and "lowest", and I did not want to change that.
>
> But the origial code supported max (highest) without min (lowest) just
> fine, which you code does not.
>
> Quite frankly, I wouldn't bother about backwards compatibility here.
> For one thing, power sensors are relatively recent, so users probably
> don't expect things to be carved in stone yet. For another, using "min"
> and "max" for "lowest" and "highest" was a bad choice in the first
> place. We have traditionally used "min" and "max" for limits, while
> what we have here are measurement extremes. Re-using "min" and "max"
> can only confuse the user. So I would use "lowest" and "highest" to
> clearly show the difference.
>
Ok, I'll luse "lowest" and "highest". Really makes much more sense.
> > However, that overlaps with "max" for the new SENSORS_SUBFEATURE_POWER_MAX.
> >
> > I've changed the highest output to to "hmax" to lift the limitation.
> > Not optimal, since there is still the average max which is also displayed
> > as "max". Please let me know if you have a better idea.
>
> Instant and average power sensors are supposed to be mutually
> exclusive, and so are their limits, so I don't see any problem here.
>
Should I use "lowest" and "highest" here as well for consistency ?
> > > (...)
> > > Do you have any more patch pending for lm-sensors? I'd like to flush
> > > the queue and then schedule the release of lm-sensors 3.3.0.
> >
> > Nothing major.
> >
> > I have a driver for MAX16065 in the queue, but it looks like that won't be detectable.
> >
> > I am also looking into support for DS1721, DS1631, and DS1731. DS1721 is supported
> > by the ds1621 driver, though auto-detection does not work if the chip is in "standard"
> > mode (the driver itself still works, though).
>
> The ds1621 driver shouldn't do device detection in the first place, it
> is hardly reliable as the devices lack an ID register.
>
> > I am waiting for samples for the other chips.
>
> I have a DS1631 somewhere in a drawer. I asked Maxim for the samples
> long ago and never got to add support for these. Shame on me. If you
> work on this, I'll be happy to help with review and testing.
>
Sounds good.
> > Once I get those, I'll test the driver and update it as/if needed. I don't have an ETA for
> > that, though.
>
> And all this is on the kernel side and not lm-sensors anyway. So let's
> get your patch finished, add a few improvements on top of it, and
> prepare for the release.
>
Mostly kernel, though I wanted to amend sensors-detect if I can for the
DSxxxx chips. But that should not hold up the new version of lm-sensors.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
