-
Notifications
You must be signed in to change notification settings - Fork 334
Add support for basv3
#482
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
base: master
Are you sure you want to change the base?
Conversation
|
@john30 What's your policy regarding register names? |
|
@GuyHarg If all goes well, and depending on how your VRC 720 is supported, |
I'd rather stick to short names for common things like heat circuit="hc", hot water circuit="hwc" etc as otherwise the names grow awfully and unnecessary long, see here https://github.com/john30/ebusd-configuration#component-type-names and for ease of use messages revealing a single value only should carry the field name "value". also I'd rather prevent reinvention of the same, i.e. if a message with a suitable name exists elsewhere: use it and rather later do refactoring overall (as is ongoing with the typespec conversion) |
@john30 Roger that! I‘ll try to keep them short.
Already the case for all my definitions. 👌
Sounds good! It’s certainly easier to refactor when naming is consistent. Lastly, as there isn’t a good generic configuration for the |
|
hi @burmistrzak, till now i was using CTLV2 config from jonesPD's repo for my regulator, which is (i think a quite early VRC70) identified as "720" in ebusd and I though I give it a try with this basv3 config. I have to say it works almost flawlessly. :) Few things I noticed:
Similar issue for Zone names.
All in all I think this could be used as a baseline even for devices identified as "720", "ctlv*", "basv*"? |
|
@kgeree Nice, thanks the detailed feedback!
As these system regulators have a lot in common, I can certainly see that happening. 😊 Edit: Did you properly clone my fork and checkout the Edit 2: Various fixes are now available in |
|
@john30 Do we already have an option to specify min/max/step values for registers? Edit: I guess this answers my first question? 👀 Edit 2: Edit 3: Just saw that support for |
|
@kgeree Were you able to checkout the |
Afaik each register can only a limited set of characters, so two registers are needed for the whole number. However, if you don't program a long enough number in the regulator, you can't read the second register because it is empty. Have you checked this scenario?
You can set a dew point offset for each heating circuit per the definition in the VRC720 manual: the minimum supply flow temperature in cooling mode is determined by the calculated dew point + this offset |
not yet...will try later. However looking at the vwzio, I think I have some more values..not sure if you can utilize something, they're all working for me (its a CSV that still needs the tempate files...) HMU and 720 looks good for the first sight (however I'm using ebusd in r/o mode yet) so can't speak for the write options... |
@kgeree The See here: ebusd-configuration/src/vaillant/08.hmu.tsp Line 791 in 08a68f1
|
|
okay...I gave it a try, have the following results: and I get "null" for the following: (but i think vwzio shall not be discussed in this PR) |
@kgeree Yes, likely.
You get
Yeah, #481 is definitely a better fit. 😊 |
right, null, but no errors. BTW is there a way to show "null" in HomeAssistant as well? then I'd get "Unknown" only if there is really an error on the ebus layer... |
@kgeree Alright, that’s good news! The
There’s probably a way to do that, but it likely requires modifying the MQTT-HASSIO configuration (value template, etc.)... I‘d say that‘s something to look at when we’ve worked out all other issues. 😉 |
|
@chrizzzp There‘re new options available in the myVaillant app! I already tried capturing the register for Upper correction value, but nothing seem to come up..? 🤔 |
As I have only temporary 'External DHW' via VR71 (heating circuit configured as external DHW), these settings don't show up for me in the app... |
@chrizzzp Hmm, you mean Circuit type: DHW on the system regulator? |
@kgeree Can you help out here? 😊 |
Correct, 'external DHW' is a rather personal nomenclature to differentiate it from the regular (internal) DHW circuit. It also operates an external valve. |
I simply don't even have "Energy management" under Settings -> Control. :) |
@kgeree Huh, strange. You do have the option Heating curve settings available, right? |
Heat curve settings I have, which is working badly...I once tried to change heat curve for 1 circuit and it changed for all 3 :|..since that I rather not touch... |
@kgeree I guess that’s the reason then. |
|
@chrizzzp What do you think about expanding the scope of this PR? |
|
I think this would make sense (if the differences between the regulators are clear). |
@chrizzzp Yea, I‘m actually not sure if there’re any registers that don’t work with all 720 models? |
|
Presumably, the currently known registers are working on all units. According to the manual there are a few additional settings for the 'v3' vs. the 'v2' units: /Installer level/ Installation configuration/Installation/DHW alternative point With the latter, I'm not sure if this corresponds to I'm not sure about the differences between the 'v1' and 'v2' regulators, though. |
afaik all values working fine for me for the (maybe v1) regulator identified as "720". |
@chrizzzp I don’t think so. |
Update regarding this little mystery: |
OK, could be. Did you change the value in the regulator and then checked e.g. |
Interesting, although from my current understanding the regulator itself does not speak EEBUS, only the gateway (VR921 etc.) does and translates everything to EBUS? So does using EEBUS still allow using SG-Ready or are they using the same regulator register(s)? I maybe wrong (I don't have any EEBUS equipment)... |
@chrizzzp Correct, only the internet gateway speaks EEBus. It’s one of the most comprehensive EEBus implementations I‘ve seen so far. There’s a handy manual available, describing what functions are supported, and how they‘re mapped to eBus.
AFAICT, you can use EEBus and SG-Ready at the same time. Similar functionality, completely different implementation. |
@chrizzzp Yup, did that. It’s not |
| @inherit(ri_1, wi_1) | ||
| @ext(0xe, 0) | ||
| model MaxRoomHumidity { | ||
| value: UIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess this would have to have a unit of %, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @burmistrzak is not active lately, I'll help out (only commenting):
Correct, unit should be %.
| model wi_1 {} | ||
|
|
||
| /** Hwc bivalence point */ | ||
| @inherit(ri_1, wi_1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any use of prohibiting reading a value to a "normal" user? IMO it would be better to allow reading always (unless it is some secret) and only limit writing according to the access level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, "normal" users should be able to read, installers should be able to write.
| @inherit(ri_1, wi_1) | ||
| @ext(0x1, 0) | ||
| model HwcBivalencePoint { | ||
| @minValue(-20) @maxValue(50) @step(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
step value of 1 should be omitted as this implicitly is the default anyway (depending on the data type though)
| @inherit(ri_1, wi_1) | ||
| @ext(0x2, 0) | ||
| model ContinuosHeating { | ||
| @minValue(-26) @maxValue(10) @step(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compared to 700.tsp the comment was omitted even though it contains valuable information. any reason for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, comment -26=off when the outside temperature falls below this threshold temperature the continuous heating function is started (off <=> function is disabled) should be included to inform about the -26=off setting.
| @inherit(ri_1) | ||
| @ext(0x10, 0) | ||
| model TariffAuxHeater { | ||
| value: UIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this value mean? any unit/value list/limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the tariff for the secondary heater (Auxiliary Heater, I assume), which is compared to the tariff of the primary heater (by the so-called Hybrid Manager, which chooses the most cost-efficient heating mode for a bivalent heater).
Unit for all tariffs could be any (e.g. cents/kWh), but have to be identical for all tariff settings.
Lower limit should be 0.
| @inherit(r_6) | ||
| @ext(0x14, 0) | ||
| @condition(Basv3.Hc3CircuitType.value, "1") | ||
| model Z1HeatingRoomTempTarget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is a desired temp, why is it not writable at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is read-only as it only shows the current desired temperature which is in effect for the zone at the current time (either set by time-progam or by manual operation mode).
See here (2nd table figure): stadid#2
However, naming it Z1HeatingRoomTempTarget could be misleading. Maybe z1ActualHeatingRoomTempDesired is better.
|
|
||
| @inherit(r_9) | ||
| @ext(0xb5, 0x55, 0xa0) | ||
| model UnknownValue_a0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unknown should be commented out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
| model HwcTimer_Config { | ||
| /** Configuration */ | ||
| @maxLength(9) | ||
| value: HEX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also long hex with unknown meaning should rather be commented out (several times)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
| @inherit(w_9) | ||
| @ext(0, 0) | ||
| @condition(Basv3.HwcEnabled.value, "1") | ||
| model HwcTimer_MondayWrite { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these have to be reworked to have read+write with the same name as desired (rather a comment to myself @john30 )
| /** timer amount */ | ||
| @inherit(r_9) | ||
| @ext(MF, 0x55, 0xa4, 0, 1) | ||
| @condition(Basv3.Hc1CircuitType.value, "1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated combined conditions could be written more nicely using a dedicated namespace with the conditions on that one, see e.g. 08.ehp.tsp#L524
|
At least for zones and heating circuits wouldn't a reusable pattern make more sense? We already know that the Controllers can support 5 zones and possibly more in some scenarios (ambisense) and it would certainly make the TSP files much more readable. @base(MF, 0x55, 0xA5, 2, 0)
model HcCommon {
HcFlowTemp : S2 @desc("Flow temperature") ;
HcFlowTempDesired : S2 @desc("Desired flow temp") ;
HcPumpStatus : UCH @desc("Pump status (on/off)") ;
HcHeatCurve : S2 @desc("Heat curve") ;
HcMaxFlowTempDesired : S2 @desc("Max desired flow temp") ;
HcMinFlowTempDesired : S2 @desc("Min desired flow temp") ;
(...)
}
@inherit(HcCommon)
@ext(0x01,0x02,0x03,0x04,0x05)
model Hc { }
(...)
model ZoneCommon {
ZRoomTemp : S2
ZRoomTempSet : S2
ZOpMode : UCH
(...)
}
@inherit(ZoneCommon)
@ext(0x01,0x02,0x03,0x04,0x05)
model Zone { } |
|
This thread is veeeeeeery long and I'm not planning on reading it all, but, I just wanted you guys to know, that I have VRC 720/2 and I can help. Here's my full dump: flexoTHERM exclusive 117/4 |
|
On the other hand, I'm wondering why my vrc 720 is called |
|
This PR aims to be the most feature-complete and robust definition for the VRC720 sensoCOMFORT line of system regulators.
It's also entirely written in TypeSpec, and already prepared to take advantage of the upcoming
rangevalidation feature for numeric values.Follow-up for #462 to keep patches manageable.