_maximumSetPointNamePrev becomes unnecessary when _maximumSetPointName gets cleared in every loop

[huangalang]

@Krellan
in commit
/Allow disabling PID loops at runtime/
the following operation was added
https://github.com/openbmc/phosphor-pid-control/blob/7c6d35d5c439196254a7ca600b2d6bc0650adf4a/pid/zone.cpp#L138C13-L138C13

and it makes this condition check always be met in every loop (and it was not original
intention)
"
else if (_maximumSetPointName.compare(_maximumSetPointNamePrev))
"

phosphor-pid-control/pid/zone.cpp

Line 276 in 7c6d35d

else if (_maximumSetPointName.compare(_maximumSetPointNamePrev))

what's more, since the condition is met in every loop
so the debug log will be printed out as well
"
std::cerr << "PID Zone " << _zoneId << " max SetPoint "
<< _maximumSetPoint << " requested by "
<< _maximumSetPointName;
"
https://github.com/openbmc/phosphor-pid-control/blob/7c6d35d5c439196254a7ca600b2d6bc0650adf4a/pid/zone.cpp#L278C8-L280C43

_maximumSetPointName.clear() in every loop is a good idea
because it's consistent with _maximumSetPoint behaviour

but it changes the logic of _maximumSetPointName.compare completly
idea1
shall we modify it accordingly?

1 change the condition
"
else if (_maximumSetPointName.compare(_maximumSetPointNamePrev))
"
=>
'else'
2 add debug flag then the debug log will not be printed all the time

idea2
or simply remove
_maximumSetPointName.clear();
then the previous logic will still work
and _maximumSetPointName actually gets updated in addSetPoint()

if you have any better idea , feel free to let us know

Alang

[Krellan]

Hmm. The intention is to not flood the log with many duplicate messages, so the logging line is only printed to the log if the requester actually changes.

If the name of the requester remains empty by the time the message is printed, that would be strange, as it would mean the entire loop had no requester of the maximum setpoint.

I will look at it, but the logic should remain the same, even if the string becomes empty. It should not be causing the line to be printed all the time. Perhaps the copying of the name to Prev should happen at the moment the logging line is printed, similar to how it is done in other places for similar loggings of maximums and such.

[huangalang]

@Krellan
any update on this ?
we suggested that
removing this line will be the simplest way
https://github.com/openbmc/phosphor-pid-control/blob/master/pid/zone.cpp#L138

[Krellan]

Sorry about that, been busy with other things.

I am OK with placing that logging line under the protection of a debug flag, for now. Only perform the logging if the user has enabled debugging. That will work around the problem of excess logging by default, for now.

[huangalang]

@Krellan
about debug flag
will you advice us to commit the dbug flag solution
or you'll do it?
so far it seems not urgent

Alang

[Krellan]

Agree, not urgent. Whenever you have time, feel free to send in a change that fixes this issue.