Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regex seems too harsh for W1100 #6

Open
pkoevesdi opened this issue Jan 15, 2023 · 10 comments
Open

regex seems too harsh for W1100 #6

pkoevesdi opened this issue Jan 15, 2023 · 10 comments

Comments

@pkoevesdi
Copy link

pkoevesdi commented Jan 15, 2023

In line 117 in benqprojector.py the regex seems too harsh for my W1100.

  • Some commands do not need a =, for instance \r*auto#\r, \r*zoomi#\r, \r*zoomo#\r, \r*up#\r, \r*down#\r, \r*left#\r, \r*right#\r, \r*enter#\r.
  • Some responses do not contain a =, for instance the answers for all the above.
  • Some responses do not contain a #, for instance the answer for \r*ltim=?#\r, see lamp time cannot be processed on W1100 #5

I'm not sure, whether this regex is only used on detecting command echos or also on responses?

@rrooggiieerr
Copy link
Owner

@pkoevesdi, the regex is used to validate the response.

According to the BenQ documentation the response should be *<command>=<value>#. On my own projector I already noticed that one command does not respond with a leading *. I will lossen the regex a bit by also making the leading # optional. Making the = optional is a bit more complex. Also, those commands seem not to really return a status, only if the command has been executed successfully.

up, down, left, right and enter all have to do with navigating the menu and the whole idea of the library is not having to use the menu.

The zoomi and zoomo are to zoom in and zoom out I guess? Is it possible to zoom to a specific point? Or how does this work? My projector only has manual zoom

@pkoevesdi
Copy link
Author

Mine also has only manual optical zoom, but also a serial-controllable digital zoom, which ist zoomI and zoomO for. In https://esupportdownload.benq.com/esupport/Projector/Control%20Protocols/PU9530/RS232%20Control%20Guide_0_Windows7_Windows8_WinXP.pdf there is also a zoom command, which is not implemented on my projector.

@rrooggiieerr
Copy link
Owner

Ok, digital zoom is actually nice to have. Does it give any feedback on zoom level? Are there fixed steps for zooming in and out. If I give the zoomi command, and then the zoomo command, do I get back to the same zoom level?

@pkoevesdi
Copy link
Author

It gives the zoom level only on the screen, not via serial. The serial response is only the command i gave itself. The steps are fixed in both directions. So, I guess, You could count internally the successfull commands and keep a variable with the current zoom level, if that's what You were thinking of.

@pkoevesdi
Copy link
Author

Ah, one more thing: enterand the navigation keys are also used to go into shift mode and then shift the picture left/right/up/down if zoomed in.

@c1em3ntchua
Copy link
Contributor

Hey @rrooggiieerr, I used to use a more primitive BenQ serial integration that was modified from the original Acer one in home assistant. They used if match := re.search(r"=(.+)#", awns): to parse the replies from the projector. I find that it worked well for my projector but I'm not sure if it would break stuff for this integration.

@rrooggiieerr
Copy link
Owner

rrooggiieerr commented Jan 16, 2023

@c1em3ntchua I agree that the library used a strict regex to validate the command responses. In my philosophy validation should be done as strict as possible and as loose as needed. The problem is some responses diverge from what is documented by BenQ. Spaces are introduce, leading * or ending # are sometimes missing.

The current regex in the library now works with above cases and works with responses which include the = sign. In most cases I'm interested in the value between the = and # for example in the following case where I like to know the current volume level:

>*vol=?#
*VOL=12#

Then there are responses which don't include the = sign. To describe what's happening I give the following example.

>*up#
*UP#

Here the > is the prompt, then the actual command *up# follows. In the code I call this the command echo. *UP# is the response of this command.

I see these response more as an acknowledgement that command has executed successfully than that I see them as a real representation of the projector state. If I'm navigating the onscreen menu using *up# I do know that the position of the selected menu item moved up but I don't know on which line of the menu I am or what the menu item is about.

What's important for detecting the supported commands is that I can receive the actual state of the projector so I can go back to that state after running the detection. For instance if I'm trying to detect the supported aspect ratios I first request the current aspect ratio, then loop trough al known aspect ratios to detect which ones are supported and then set the projector back to it's original aspect ratio. If I would like to detect if zoomi and zoomo is supported without being able to request the current zoom level the projector might end up on a different zoom level than before the detection process.

I hope this makes a bit more clear how the communication with the projector works and how I have designed the library to work accordingly. And also why detecting all the supported features is not really 100% possible.

Then about your proposed regex, that one is actually more strict than the one currently implemented in the library because it expects an ending # and also requires the =.

@rrooggiieerr
Copy link
Owner

I created a function to send raw commands. This way the mentioned commands but also future commands which are not (yet) supported by the library can still be send. The HA integration will have a service to send raw commands to the projector so that's also covered.

Do you think this is a workable solution?

I also improved the examine feature of the cli, give it a try

@rrooggiieerr
Copy link
Owner

Hi @pkoevesdi , I'm guessing the items mentioned in this issue are now resolved/covered so I can close the issue?

Please reopen if you think that's not the case.

Thanks!

@pkoevesdi
Copy link
Author

Similar to #7 (comment):
On first check it works on my projector. But I'll go deeper into it and make a Pull Request, If everything works for my W1100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants