Lg lcd

[pkheav]

No description provided.

[kimburgess]

It will be much safer to work with this as it's enum value rather than in string form.

[pkheav]

Yeah I tried using the enum values for this but because some of the commands use the same letter/value, then I can't actually distinguish between their responses just with from the first character in the response string.

Pretty much if there's duplicate values in an enum, it'll always default to the one defined first so AutoOff in this case https://play.crystal-lang.org/#/r/9tt5. Let me know if you have a better idea of dealing with this.

Also, this comments try to explain that

drivers/drivers/lg/displays/ls5.cr

Lines 223 to 225 in 3dea9fe

	# Both the responses for contrast/no_signal_off will have data[0] == 'g'
	# Same thing for auto_off/pm_mode with data[0] == 'n'
	# We will try using the task name to actually distinguish between these pairs

[kimburgess]

Ah I see. Looking at the protocol, commands are two bytes. Rather than fudging things on the parsing it will be more reliable to rework the way this is modelled to capture this.

One approach that could work is to pack the two command bytes (UInt8's) into a single UInt16. This will then let the commands be represented, without ambiguity and also ensure that everything needed to serialise and deserialise them is define clearly in one spot.

enum Command : UInt16
  Power    = 0x6b61 # k a
  SetInput = 0x7962 # x b
  # etc
  
  def self.[](data1 : Char, data2 : Char)
    self.new((data1.ord.to_u16 << 8) ^ data2.ord.to_u8)
  end
end
 
p! Command['k', 'a']
# => Power

https://play.crystal-lang.org/#/r/9ufd

The only slightly annoying thing is I'm not aware of a way to get Char#ord values from within a macro or force compile time eval of these to make the actual value declarations a little neater. E.g. ideally this would be:

enum Command : UInt16
  Power = bitpack('k', 'a')
  # ...
end

Tagging @stakach and @caspiano for other thoughts on this one too as defining a clean way to express multi-byte Enum's is a common problem worth solving.

[kimburgess]

P.S. see crystal-lang/crystal#9830 for a proposal to solve this at the language level. Hopefully it's generally useful, however if there's no interest there can look at adding something to the driver framework.

[pkheav]

Hey @kimburgess , if I do switch over to packing 2 bytes into a command like NoSignalOff = 0x6667 # fg, I'm not sure how I'll be able to use the enum to parse responses.

So the command for no_signal_off(false) is "fg 01 00\r" and the response is "g 01 OK00x". Since Command::NoSignalOff is representative of the two bytes which are only present in the command that's sent and the response contains only one of these bytes "g", this will make it difficult if not impossible to figure out what command the response is actually referring to from just using the enum.

[kimburgess]

Yep - I see what you mean. In cases like this where the protocol does not provide the ability to uniquely distinguish the response from the response data alone, you may want to define response parsing as a callback block, in place of the single received method.

Take a look at "send and callback" here.

[kimburgess]

What special about system k? Can this be given some meaning from either encoding this in a value, or at a minimum referencing in a comment?

[pkheav]

drivers/drivers/lg/displays/ls5.cr

Lines 281 to 285 in 3dea9fe

	# From manual
	# [Command1]: identifies between the factory setting and the user setting modes.
	# Default c1 to 'k' which appears to be for user settings
	# and which most commands use (e.g. Mute, Screen off, Volume, Brightness)
	private def do_send(command : Command, data : Int, c1 : Char = 'k', **options)

[kimburgess]

Ditto here for s.

[pkheav]

drivers/drivers/lg/displays/ls5.cr

Lines 286 to 289 in 3dea9fe

	# Command::PmMode and Command::AutoOff both are equal to 0x6E == 'n'
	# However, PmMode has c1 == 's' while AutoOff has c1 == 'm'
	# So this is how we can differentiate whether the command we want to send is PmMode
	if command.pm_mode? && c1 == 's'

[kimburgess]

Why the conditional here?

[pkheav]

No idea why actually as I've copied this over from the ruby driver. I can get rid of this but it might have been there for a reason?

Not exactly of the difference between communicating with a device that's hooked up via rs232 compared to one that's not regarding this bit of code as well which I also carried over from the ruby driver

drivers/drivers/lg/displays/ls5.cr

Lines 145 to 160 in 19d0468

	if @rs232
	power?
	if self[:hard_power]?.try &.as_bool
	screen_mute?
	input?
	volume_mute?
	volume?
	end
	elsif self[:connected]?.try &.as_bool
	screen_mute?
	if @id_num == 1
	input?
	volume_mute?
	volume?
	end

[stakach]

I think from a Crystal engine point of view, self[:connected] == JSON::Any.new(true / false)
So JSON::Any.new(false) evaluates to true so need to use .raw or move it into the expected type via as_bool (will raise an error if an Int etc)

The real answer is to track @connected state via an instance variable and just update the public state as required (much more performant too)

[pkheav]

Thanks @stakach , I've made these commits.

d5c37cb

2d7d4fc

[kimburgess]

Yep - I see what you mean. In cases like this where the protocol does not provide the ability to uniquely distinguish the response from the response data alone, you may want to define response parsing as a callback block, in place of the single received method.

Take a look at "send and callback" here.

[pkheav]

Ah, I did not know about the send and callback which yeah is useful for cases like this. Thanks @kimburgess , I've made this commit c3131c4