_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)) " https://github.com/openbmc/phosphor-pid-control/blob/7c6d35d5c439196254a7ca600b2d6bc0650adf4a/pid/zone.cpp#L276

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.