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

Enhance support for Vasco and ClimaRad HRUs #133

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

silverailscolo
Copy link
Contributor

Proposed additions to process messages from a Vasco D60 HRU (2022), a ClimaRad VenturaV1x HRU (2021) and a ClimaRad MiniBox fan that I have installed in my home.

I have ramses_rf communicating via ramses_esp on an ESP32S3 board with a C1101..

Not all commands as entered in the ramses-rf CLI cause a change on the units, but at least reporting from the units works. The message contents are not shared by the manufacturer and especially for the Ventura they customized them. The PCBs are all Airios OEM.
Many messages from the units and their remotes are rejected by ramses-rf, so I cannot run Home Assistant with ramses-cc at the moment.

Hope to have made it a bit easier to add suport for these models in ramses-rf. The typed stuff is a bit too much so I cheated somewhat there. Fixed some typos in comments across your module while browsing.
Added a packet log below. I can provide more info upon request.

Cheers, Egbert

packet3.log

@zxdavb zxdavb changed the title Vasco and ClimaRad HRU Enhance support for Vasco and ClimaRad HRUs Oct 11, 2024
@zxdavb
Copy link
Owner

zxdavb commented Oct 12, 2024

Thanks for your contribution, I am looking forward to merging it in.

You'll see that the PR did not pass CI, are you running the pre-commit, mypy and pytest?

pre-commit run -a
mypy
pytest

... all these should pass before submitting a PR.

Second, the PR should be as small as possible, and atomic. If something could be split off as a separate PR, generally it should be: the spelling typos are once such example.

@zxdavb
Copy link
Owner

zxdavb commented Oct 12, 2024

OMG, just adding codespell - is full of typos - I will add codespell, fix many of these in my own PR.

(you'll have to rebase)

Copy link
Owner

@zxdavb zxdavb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a first step. We can look again, once I have time to understand the detail.

src/ramses_rf/binding_fsm.py Show resolved Hide resolved
src/ramses_rf/binding_fsm.py Show resolved Hide resolved
src/ramses_rf/binding_fsm.py Outdated Show resolved Hide resolved
src/ramses_tx/schemas.py Show resolved Hide resolved
src/ramses_tx/ramses.py Show resolved Hide resolved
Comment on lines 54 to 61
ITHO__ = "itho"
NUAIRE = "nuaire"
ORCON_ = "orcon"
VASCO_ = "vasco"
CLIMARAD = "climarad"

TEST_SUITE_300 = [
# { # THM to CTL: FIXME: affirm is I|1FC9|07, not I|1FC9|00
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add some test bindings of these systems. Look at ./tests/bindings/hvac/*

Copy link
Contributor Author

@silverailscolo silverailscolo Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bindings are further down the To Do list. Have to climb on top of our bathroom to push a button on the MiniBox...
Took these lines out of the 22F1 PR #136 .

@zxdavb zxdavb self-requested a review October 12, 2024 09:20
@silverailscolo
Copy link
Contributor Author

silverailscolo commented Oct 12, 2024

Thanks for the educational pointers.
Running mypy (latest v 1.11.2) - as well as my IDE (IntelliJ) - on current HEAD master already gives 27 errors like:

src/ramses_cli/client.py:449: error: "Code" has no attribute "values"  [attr-defined]

On my PR branch I get 8 more, but still.

As a novice in mypy I wonder if you happen to know a fix/setting? It it a version thing perhaps (python 3.12 here).

@zxdavb
Copy link
Owner

zxdavb commented Oct 13, 2024

Running mypy (latest v 1.11.2) - as well as my IDE (IntelliJ) - on current HEAD master already gives 27 errors like:

I can reproduce this... the solution is to pip install -e . like so:

python3 -m venv venv
source venv/bin/activate
pip install -e .
pip install -r requirements_dev.txt

It is not immediately clear to me why the existing pyproject.toml isn't sorting it out without the -e.

@zxdavb
Copy link
Owner

zxdavb commented Oct 13, 2024

OK, I have had a deeper look now, and here's a way forward...

It appears you have updated the parsers for at least the following command codes:

  • 12A0, 2210, 22F1, 22F3, 22F4, 31DA & 3200

I am going to ask you to submit multiple PRs - one for each code.

You will also need to add packets to the corresponding tests. For example, look at:

  • tests/tests/parsers/code_22f1.log

In addition, it will be very helpful if you have a log of your remotes/sensors binding to your fans. This will help with faking.

And also a 10E0 packet for all of your devices that emit one.

  • tests/tests/parsers/code_10e0.log

@silverailscolo
Copy link
Contributor Author

It is not immediately clear to me why the existing pyproject.toml isn't sorting it out without the -e.

For my own reference, learned this on Stackoverflow:

To execute a local package containing absolute or relative imports without needing to install it. This use case is detailed in PEP 338 and leverages the fact that the current working directory is added to sys.path rather than the module's directory. This use case is very similar to using pip install -e. to install a package in develop/edit mode.

Sorting out my Python setup now.

@silverailscolo
Copy link
Contributor Author

submit multiple PRs - one for each code.

Perfect approach, thanks. TBC

src/ramses_tx/parsers.py Outdated Show resolved Hide resolved
@zxdavb
Copy link
Owner

zxdavb commented Oct 13, 2024

You can leave this PR here for now - the TODO list is useful.

As I said in #134 - start with the codes, one PR per code - a number of smaller PRs is much easier to get merged (for both of us).

You don't have to do one at a time, you can have several codes on the go at once, but start with a simple one, and you know what to expect to get your PR merged (e.g. including your new payloads in the test suite).

@silverailscolo
Copy link
Contributor Author

silverailscolo commented Oct 23, 2024

All open PR's in the above list are now ready for your review

@silverailscolo
Copy link
Contributor Author

silverailscolo commented Oct 25, 2024

Current master reports these codes as errors still:

  • 12A0 Payload doesn't match
  • 2210 report this packet RP 2-4x/minute
  • 2E10 report this packet
  • still watching...

@silverailscolo
Copy link
Contributor Author

@zxdavb Any chance we can see these changes in a ramses_cc update before year's end?

@zxdavb
Copy link
Owner

zxdavb commented Nov 27, 2024

I am sorry, but it looks unlikely I'll have that much time.

What is the feature you're wanting most?

@silverailscolo
Copy link
Contributor Author

That would be 10E0, 1298 and 22F1 for a start.

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

Successfully merging this pull request may close these issues.

2 participants