Skip to content

Comments

Feature/chad 12650/md exploring#1

Draft
matthewdehaven wants to merge 29 commits intomainfrom
feature/CHAD-12650/md-exploring
Draft

Feature/chad 12650/md exploring#1
matthewdehaven wants to merge 29 commits intomainfrom
feature/CHAD-12650/md-exploring

Conversation

@matthewdehaven
Copy link
Owner

No description provided.

local min_temp_c = celsius_setpoint_temperature(args.precision1, args.min_value, args.scale1)
local max_temp_c = celsius_setpoint_temperature(args.precision2, args.max_value, args.scale2)

device:emit_event_for_endpoint(cmd.src_channel, capabilities.thermostatHeatingSetpoint.heatingSetpointRange(
Copy link

Choose a reason for hiding this comment

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

so which capability you send this event for (thermostatHeatingSetpoint or thermostatCoolingSetpoint) will depend on cmd.args.setpoint_type. Specifically, we'll only be looking for ThermostatSetpoint.setpoint_type.HEATING_1 and ThermostatSetpoint.setpoint_type.HEATING_2

Copy link
Owner Author

Choose a reason for hiding this comment

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

Do you mean ThermostatSetpoint.setpoint_type.COOLING_1 instead of ThermostatSetpoint.setpoint_type.HEATING_2? I don't see a reference to HEATING_2 anywhere.

Copy link

Choose a reason for hiding this comment

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

I did in fact mean that.

end
end
local args = cmd.args
--- FIXME - MAD - Verify scale1 is associated with min_value and scale2 is associated with max_value
Copy link

Choose a reason for hiding this comment

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

this is correct

Comment on lines 106 to 109
local function concrete_setpoint_temperature(precision, value)
local result = value / (10 ^ precision)
return result
end
Copy link

Choose a reason for hiding this comment

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

we have a little bit of magic that we do in our generated z-wave libraries so that fields encoded with precision and size are just automatically translated to numbers, so you should just be able to use cmd.args.min_value and cmd.args.max_value without having to do the manual precision calculation.

Comment on lines 411 to 414
size1 = 4,
scale1 = ThermostatSetpoint.scale.CELSIUS,
precision1 = 2,
min_value = 722,
Copy link

Choose a reason for hiding this comment

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

part of that magic includes the serialization, so you should be able to get by with just omitting sizeX and precisionX and just putting 72.2 for the min_value and it should "just work"

local capability_constructor = nil
if args.setpoint_type == ThermostatSetpoint.setpoint_type.HEATING_1 then
capability_constructor = capabilities.thermostatHeatingSetpoint.heatingSetpointRange
else
Copy link

Choose a reason for hiding this comment

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

you should specifically check for COOLING_1 since it is possible (if unlikely) that some other controller on the network inquires about the limits for one of the other setpoint types we don't support.

Comment on lines 107 to 108
local min_temp_c = args.min_value
local max_temp_c = args.max_value
Copy link

Choose a reason for hiding this comment

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

rename these, since they're not guaranteed to be in C

They are not guaranteed to be in celsius
To account for the case a different controller on the network inquires about an unsupported setpoint
@matthewdehaven
Copy link
Owner Author

@greens can I open a pull request on the main repo? I created this fork here because I was just exploring the code, but it seems to me it may be good to move over to the main repo now.

@greens
Copy link

greens commented Apr 23, 2024

@matthewdehaven Yep. Go ahead! I think you could actually just change the target of this PR to be the main branch of the origin repo, or you can open a new PR.

@matthewdehaven
Copy link
Owner Author

@greens

I couldn't change the target repository of this merge request, so I just opened a new one here: SmartThingsCommunity#1344

I can't request reviewers, though. I'm guessing I'm not a member of the correct group.

I need some help with review request part of the PR process.

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