Add new features

[iloveicedgreentea]

Hey, I am the maintainer for another JVC integration

https://github.com/iloveicedgreentea/jvc_projector_python

https://github.com/iloveicedgreentea/jvc_homeassistant

I have archived it since yours is in core already. I plan on extending the core version to support new sensors and other entities that my custom component had. As a precursor, I wanted to add some features to the library

Summary of changes

1. Overhauled sending commands to use human readable commands
2. Update state function to be modular
3. General improvments
4. Lowered throttling to speed up state updates (maybe this should be configurable). I found that anything over 0.2s is pretty safe. I have had users report lockups if it is too fast though. However, updates should be under 30s otherwise HA will start complaining.
5. added helper functions to transform values to human readable ones like version, model, mac, etc
6. support various functions for home assistant
7. HA PR - https://github.com/home-assistant/core/pull/131320/files

I updated some tests and made sure all of these worked with my NZ7. I like how you set up the formatter with regex for parsing commands, that is very slick.

Please let me know what you think

[iloveicedgreentea]

I will add the new handshake procedure for new models tomorrow as well

[iloveicedgreentea]

Added the new auth protocol and a new way to send commands. It uses a send_command function that can send CMD consts. This will allow home assistant to send commands like set the laser_power to high (added a test for that). I would appreciate if someone could review this so I can get started on the home assistant part. I want to extend sending commands, add sensors, add binary sensors, etc

I updated the formatters to use consts so the keys can be referenced in commands

[iloveicedgreentea]

Heres some output examples

{'model': 'ILAFPJ-B5A3', 'mac': 'E0DADC0A269A'}

Current state:
model: NZ7 <- model code lookup 
version: 3.0.0 <- version is semver now
mac: E0:DA:DC:0A:26:9A <- format mac
power: on
input: hdmi2
source: signal
picture_mode: user1
low_latency: off
laser_power: high
anamorphic: off
installation_mode: mode5
hdr: none
hdmi_input_level: standard
hdmi_color_space: auto
color_profile: auto
graphics_mode: high-res
color_space: rgb
eshift: off
clear_motion_drive: off
motion_enhance: low
laser_value: 100 <- this is normalized to 0-100
laser_time: 2195 <- hours
laser_dimming: auto3
hdr_processing: None
hdr_content_type: auto

I added a functional test so you can check it out as well

[iloveicedgreentea]

Any chance this could get reviewed?

[jmery]

@SteveEasley Would it be possible to get this reviewed and merged? This add-on is broken for all of the new JVC models due to a password format change. @iloveicedgreentea has that fixed above so it would be great to implement.

[iloveicedgreentea]

It’s been a while so I might just have to fork this and make a PR to home assistant to use the fork

[SteveEasley]

Hey sorry, just noticed this request after ironically just pushing up some changes to fix the auth issue. Not sure why I didn't get a git notification of the PR until I pushed up my change.

@iloveicedgreentea I notice our auth code is a little different. I think your implementation might be including the _ in the sha256 encode, which is a mistake? I also added a test to verify the functionality (I dont have a newer projector to test though).

The const changes look like a good thing. But I do want to take a day or so to test the other new commands on my own projector. I have some concerns about the fast refreshes and bogging down the projector with the new traffic. But might be ok.

How about we split this up into two HA PRs? I will get my auth changes pushed through, and lets work on the new commands as a separate (but concurrent) change. Feel free to give feedback on my changes though (05d8e86)

[iloveicedgreentea]

Hey sorry, just noticed this request after ironically just pushing up some changes to fix the auth issue. Not sure why I didn't get a git notification of the PR until I pushed up my change.

@iloveicedgreentea I notice our auth code is a little different. I think your implementation might be including the _ in the sha256 encode, which is a mistake? I also added a test to verify the functionality (I dont have a newer projector to test though).

The const changes look like a good thing. But I do want to take a day or so to test the other new commands on my own projector. I have some concerns about the fast refreshes and bogging down the projector with the new traffic. But might be ok.

How about we split this up into two HA PRs? I will get my auth changes pushed through, and lets work on the new commands as a separate (but concurrent) change. Feel free to give feedback on my changes though (05d8e86)

Sure sounds good I copied the implementation from my library which worked for users but could have added an extra underscore there. I don’t think the underscore is supposed to be included. I can rebase without the auth changes when it’s merged

[SteveEasley]

The auth change is already merge to main here (will be good to have eyes for PRs in the future though!). So feel free to rebase when you are ready. I am starting to dig into your changes now, with my projector.

[SteveEasley]

@jmery You should actually be able work around the issue until we get this into HA by doing the password encode yourself.

In fact this might be a good test of our future encoding logic, since you would be manually doing exactly what's being done in the library.

1. Add a password to your projector if it doesn't have one already (e.g. 1234567890).
2. Now SHA256 encode that password + the string JVCKWPJ into an online SHA256 calculator https://www.google.com/search?q=sha256+encode+online. (e.g. 1234567890JVCKWPJ is encoded as cc3054cf7c701a32a883bf45a7b19cbf7f62db778208b3c058e0c6dde886996f).
3. Use that encoded password as your password in the JVC Projector integration of HomeAssistant. I think you might have to remove the integration first, since there is no way to change or edit the password afterward.

Thanks!

[SteveEasley]

Today in main I replaced these lint/format tools with ruff (same as HA uses), and added a pre-commit hook for it.

[iloveicedgreentea]

Thanks I will take a look at these this week

[jmery]

@jmery You should actually be able work around the issue until we get this into HA by doing the password encode yourself.

In fact this might be a good test of our future encoding logic, since you would be manually doing exactly what's being done in the library.

1. Add a password to your projector if it doesn't have one already (e.g. 1234567890).
2. Now SHA256 encode that password + the string JVCKWPJ into an online SHA256 calculator https://www.google.com/search?q=sha256+encode+online. (e.g. 1234567890JVCKWPJ is encoded as cc3054cf7c701a32a883bf45a7b19cbf7f62db778208b3c058e0c6dde886996f).
3. Use that encoded password as your password in the JVC Projector integration of HomeAssistant. I think you might have to remove the integration first, since there is no way to change or edit the password afterward.

Thanks!

This doesn't appear to work. I reset the PJ's network password. Adding the integration via the HA UI constantly returns "Invalid Authentication".

[SteveEasley]

Hmm that’s concerning since that’s the same process we would use in the code.

Would you mind trying with a test password and share it and the sha256 value you put into the HA integration with me?

And/or try it the exact values I gave above. 1234567890 for the projector password and cc3054cf7c701a32a883bf45a7b19cbf7f62db778208b3c058e0c6dde886996f for the HA integration password.

[jmery]

Hmm that’s concerning since that’s the same process we would use in the code.

Would you mind trying with a test password and share it and the sha256 value you put into the HA integration with me?

And/or try it the exact values I gave above. 1234567890 for the projector password and cc3054cf7c701a32a883bf45a7b19cbf7f62db778208b3c058e0c6dde886996f for the HA integration password.

I reset it to your exact example from the original password. I don't see any instructions from JVC requiring any kind of reboot or anything for the password to be effective.

[SteveEasley]

Ok figured this out. This was actually an unrelated bug thats always been there. Passwords over 10 characters were getting truncated to 10 characters, so it wont except the long SHA :( I submitted a fix for this here home-assistant/core#126581. But by the time thats fixed, the SHA256 PR (home-assistant/core#126453) should be live at the same time, so should be able to use your regular password.

Thanks for taking the time to try it, squashed a bug at least!

[jmery]

Ok figured this out. This was actually an unrelated bug thats always been there. Passwords over 10 characters were getting truncated to 10 characters, so it wont except the long SHA :( I submitted a fix for this here home-assistant/core#126581. But by the time thats fixed, the SHA256 PR (home-assistant/core#126453) should be live at the same time, so should be able to use your regular password.

Thanks for taking the time to try it, squashed a bug at least!

2024.9.3 shipped overnight. I just installed and can confirm the issue is fixed. Thanks so much for the quick fix and happy to have helped a little :-)

[iloveicedgreentea]

I rebased to main, used your connect code, and fixed the rest of the comments. Please let me know what you think

[SteveEasley]

Cool will look at this over the next couple of days.

[iloveicedgreentea]

pushed a fix to improve sending info commands and fix version processing for unknown string

[SteveEasley]

fyi looking good. I am assuming we will leave this open until you are done on the HA side and ready for a PR on that side.

[iloveicedgreentea]

fyi looking good. I am assuming we will leave this open until you are done on the HA side and ready for a PR on that side.

Yeah I started working on that in core. I will try to get it plugged in soonish

[iloveicedgreentea]

@SteveEasley I made a draft PR for HA core here https://github.com/home-assistant/core/pull/131320/files

I added sending commands and the new sensors. I need to do more testing but at first glance it all works. I am installing the lib from git (pulling changes into my core devcontainer) with

pip install  /workspaces/pyjvcprojector/

Makes it easy to iterate

LMK what you think. I will probably disable some sensors by default because the core people will complain about it. I also want to make it easier to see what commands can be sent like "laser_power, high".

I also updated the tests.

[iloveicedgreentea]

I pushed changes to automate generating commands and for HA I added more selects and improved sensors and all that. Will do more testing tomorrow. 99% code coverage too

---------------------------------------------------------------------------------------
homeassistant/components/jvc_projector/__init__.py           31      1    97%   50
homeassistant/components/jvc_projector/binary_sensor.py      29      0   100%
homeassistant/components/jvc_projector/config_flow.py        62      0   100%
homeassistant/components/jvc_projector/const.py               5      0   100%
homeassistant/components/jvc_projector/coordinator.py        31      0   100%
homeassistant/components/jvc_projector/entity.py             17      0   100%
homeassistant/components/jvc_projector/remote.py             44      1    98%   71
homeassistant/components/jvc_projector/select.py             33      0   100%
homeassistant/components/jvc_projector/sensor.py             25      0   100%
---------------------------------------------------------------------------------------
TOTAL                                                       277      2    99%

[iloveicedgreentea]

Updated model and version display as well. I tested the selects, it all seems to work. Please test and let me know. You will probably have to edit your manifest to have this.

{
  "domain": "jvc_projector",
  "name": "JVC Projector",
  "codeowners": ["@SteveEasley", "@msavazzi"],
  "config_flow": true,
  "documentation": "https://www.home-assistant.io/integrations/jvc_projector",
  "integration_type": "device",
  "iot_class": "local_polling",
  "loggers": ["jvcprojector"],
  "requirements": ["/workspaces/pyjvcprojector/"]
}

[iloveicedgreentea]

Okay I think im done for now. I added mypy and enabled pre-commit and fixed all the errors. I uploaded a test package to pyjvcprojector_test so if anyone wants to test you can add this branch as a custom component and set the manifest to pyjvcprojector_test==1.1.2. You will need to add a version field set to anything too

[iloveicedgreentea]

@SteveEasley ive been running this in my HA instance and its been good the only issues I see are occasional timeouts (just while sending commands not reading state)

I suspect its an issue of trying to read an empty buffer which suggests a syncing issue but not sure. Not sure if this ever happened before my changes. In my custom component I implemented a PriorityQueue which allowed user commands to have immediate access while refresh commands took a back seat. This gave you strict command ordering, priority, and a queue system. let me know if you are interested in that I can implement it

also to be clear the timeout only happens when sending commands so I think its related to synchronizing writing and reading

[SteveEasley]

Hey, sorry for the delay. Should be diving into this by the end of the week.

[iloveicedgreentea]

Cool no worries. So far I haven’t run into any issues besides occasional warning for timeouts. I don’t send a lot of commands though. I suspect we may need a command queue regardless

[Sdub76]

Hey, sorry for the delay. Should be diving into this by the end of the week.

Any update? I'm waiting for this to merge before switching to the official JVC integration, since I use installation mode in my automations. Was hoping to make the update over the holidays. Thanks!

[SteveEasley]

Any update? I'm waiting for this to merge before switching to the official JVC integration, since I use installation mode in my automations. Was hoping to make the update over the holidays. Thanks!

Yep I provided some feedback on the HA side in home-assistant/core#131320 last weekend. It was working for me for the most part.