Feedback on a bit of Lua code

[FearTheDentist]

I'm almost done writing a serial plugin for Pioneer PDP plasma displays and wanted some input on one piece of code. The unit only accepts absolute volume adjustment over the serial connection, not relative. I wrote a little bit of code so this could still be handled by a conventional volume +\- control. It works perfectly, but given feedback I've received on previous work am concerned that this may not be a very efficient way to do this.

Also- I've set it up to poll the unit every 60 seconds. Chars 5-7 in the resulting data is the current volume. I'd like to pass this to the volume function as a starting point (currently I arbitrarily picked 25), but am having trouble figuring out how to capture these 3 chars. This is really bugging me because I figured this out once before but cannot seem to recreate it...
The data is in the format QSUXXX..... where XXX is the volume.

What do you think?



VolUp = function (self)
PioneerPDPVol = PioneerPDPVol or 25
if PioneerPDPVol == 42 then
PioneerPDPVol =42 --max. volume
else
PioneerPDPVol = PioneerPDPVol + 1
end
if string.len(PioneerPDPVol) == 1 then
PioneerPDPVol = '0' .. PioneerPDPVol
end
return self:SendCommand ('01VOL0'..(PioneerPDPVol))
end,

VolDown = function (self)
PioneerPDPVol = PioneerPDPVol or 25
if PioneerPDPVol == '00' then
PioneerPDPVol =0 --min. volume
else
PioneerPDPVol = PioneerPDPVol - 1
end
if string.len(PioneerPDPVol) == 1 then
PioneerPDPVol = '0' .. PioneerPDPVol
end
return self:SendCommand ('01VOL0'..(PioneerPDPVol))
end,

[Rob H]

I'm not very keen on mixing strings and numbers like you are in that code - it looks like the volume ends up as being both at various times - I'd recommend sticking to a single type (preferably a number).

Also you're doing



if PioneerPDPVol == 42 then
PioneerPDPVol =42 --max. volume
else
PioneerPDPVol = PioneerPDPVol + 1
end
the first branch of that does nothing and will actually fail if PioneerPDPVol is '42'

On to your question:

Suppose the string QSUxxx is in a variable called data then you'd use something like


local _, _, vol = string.find(data, 'QSU(%d%d%d)')

[FearTheDentist]

if PioneerPDPVol == 42 then
PioneerPDPVol =42 --max. volume
else
PioneerPDPVol = PioneerPDPVol + 1
end

Hmm..this is intended to prevent PioneerPDPVol from exceeding 42, which is the top end of the range for the unit. It does work as intended and doesn't kick out an error. Likewise, the other half prevents PioneerPDPVol from dropping <0. I assume there's a better way to do this ie. define the variable range as 0-42, but I haven't figured out how.

[Rob H]

As I said it won't work if PioneerPDPVol is '42' rather than 42, but that may never happen. It's still a bad idea to mix strings and numbers in the same variable.

Looking further at the code I don't think it will ever be a string unless it's actually a single digit so you're probably safe.

I'd still use another variable to be on the safe side and would use something like


return self:SendCommand ('01VOL0'..string.format('%02d', PioneerPDPVol))
which will left pad the value with zeroes in a field width of 2.

Then you could keep PioneerPDPVol as a number throughout.

[FearTheDentist]

Ah- that's what I was looking for. I thought there probably was a better way, but unfortunately I'm kind of learning as I go... I get string vs. numbers conceptually but I frequently get confused with the syntax as I'm trying to write the code. When something works it's usually after several rounds of trial and error (ie. it's a mistake to assume I know what I'm doing. I think the phrase is "Knows just enough to be dangerous" :D). That's why I keep pestering you for help. Thanks!!!!

[quixote]

What about using 'tonumber'? Seems a little more simple, no?

[Rob H]

I don't see how tonumber() would help?

[quixote]

To make sure your not comparing strings to numbers and vice versa.

[FearTheDentist]

Almost there, but a little confused on how local vs. global variables work. I gather we'd rather not go tossing endless global variables about, so want to keep them local much of the time. Via trial and error, I am under the impression that a local variable is good only in the context of the function in which it is created ie. if local X is defined in function A, it cannot also be called from function B. Is this correct?

The reason I ask is I've been trying to implement "local _, _, vol..." from above. When I put this in any function other then ReceiveResponse, it returns a nil value. When I put it in ReceiveResponse it returns the current volume as required, but I can't call this from the VolUp or VolDown functions. If I make it a global variable it works just fine. I know I'm missing a concept here, but for the life of me I can't figure out what I'm doing wrong.

Also- is there a way to tell the script to poll the unit when it is initialized? Otherwise, the variable 'vol' is not populated and remains nil until the unit is polled after 60 seconds.

The ironic thing is I don't even use the built-in speakers, I'm just doing this to make sure the plug-in is comprehensive...

here's the relevant portions of my revised code:


VolUp = function (self)
PioneerPDPVol = PioneerPDPVol or vol
if PioneerPDPVol == 42 then --max. volume
else
PioneerPDPVol = PioneerPDPVol + 1
end
return self:SendCommand ('01VOL0'..string.format('%02d', PioneerPDPVol))
end,

VolDown = function (self)
PioneerPDPVol = PioneerPDPVol or vol
if PioneerPDPVol == 0 then --min. volume
else
PioneerPDPVol = PioneerPDPVol - 1
end
return self:SendCommand ('01VOL0'..string.format('%02d', PioneerPDPVol))
end,

ReceiveResponse = function (self, data, code )
if math.band (code,serial.RXCHAR) and data then
print ("PioneerData",(data))
end

if math.band(code, serial.NORESPONSETIMEOUT) > 0 then
self.ErrorCount = (self.ErrorCount or 0) + 1
if self.ErrorCount > 5 then
self.ErrorCount = 0
self:Reset()
gir.LogMessage(self.Name, "Reset Com" .. (self.ComPort),3)
end
end --resets the com port in case of communications error following resume from suspend
_, _, vol = string.find(data, "QSU(%d%d%d)")
serial.Classes.Queued.ReceiveResponse (self,data,code)
end,

[Rob H]

I'd suggest making vol a field of the object itself ie use self.vol instead

[FearTheDentist]

I'd suggest making vol a field of the object itself ie use self.vol instead

I'm not sure I follow you?

[Rob H]

ie use


PioneerPDPVol = PioneerPDPVol or self.vol

and


_, _, self.vol = string.find(data, "QSU(%d%d%d)")

Actually you should probably convert self.vol into a number too ie


_, _, self.vol = string.find(data, "QSU(%d%d%d)")
self.vol = tonumber(self.vol)

[FearTheDentist]

Done, works great! One last question on this one- Is there an easy way to tell the script to poll the unit when the script is initialized? Otherwise self.vol isn't populated until the unit is polled after a minute and thus the volume control doesn't work for the first minute of operation.

Thanks again for all your help!

[Rob H]

How come it's polling after a minute?

[FearTheDentist]

How come it's polling after a minute?

When the unit is initialized it returns PioneerPDPData: 0. The first time it is polled @ 60sec it returns the status codes it should. My DVD player behaves the same.

[Rob H]

What's doing the polling?

[FearTheDentist]

I wrote it into the plugin ("borrowed" the code from the DenonReceivers.lua script):


Initialize = function (self)
gir.LogMessage(self.Name, 'Communications OK',3)
self.Status = "Communication OK"
self:StartPollingState ()
return serial.Classes.Queued.Initialize (self)
end,

StartPollingState = function (self)

self.PollingStateTimer = gir.CreateTimer (
nil,
function ()
self:PDPStatus ()
print (data)
end,
nil,
1)
self.PollingStateTimer:Arm (60000)
end,

StopPollingState = function (self)
_ = self.PollingStateTimer and self.PollingStateTimer:Destroy ()
self.PollingStateTimer = false
end,