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.

Sign up for GitHub

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

Fix the frequency of AC temperature update #292

Merged
merged 5 commits into from
Jun 3, 2024
Merged

Fix the frequency of AC temperature update #292

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
183f7a6
fix: AC accurate temperature update using control endpoint
aabdellah May 30, 2024
5759c9d
fix: Only send control requests for online ACs
aabdellah May 30, 2024
fe2fc8a
fix: Set default threshold temperature step to 1
aabdellah May 30, 2024
037b5d3
fix: Remove console statements
aabdellah May 30, 2024
b27b872
Revert "fix: Set default threshold temperature step to 1"
aabdellah Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 11 additions & 7 deletions src/devices/AirConditioner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,14 @@ export default class AirConditioner extends baseDevice {
// send request every minute to update temperature
// https://github.com/nVuln/homebridge-lg-thinq/issues/177
setInterval(() => {
this.platform.ThinQ?.deviceControl(device.id, {
dataKey: 'airState.mon.timeout',
dataValue: '70',
}, 'Set', 'allEventEnable').then(() => {
// success
});
if (device.online) {
this.platform.ThinQ?.deviceControl(device.id, {
dataKey: 'airState.mon.timeout',
dataValue: '70',
}, 'Set', 'allEventEnable', 'control').then(() => {
// success
});
}
}, 60000);
}

Expand Down Expand Up @@ -661,7 +663,7 @@ export default class AirConditioner extends baseDevice {
type: ValueType.Range,
min: 0,
max: 0,
step: 0.01,
step: 1,
Copy link
Owner

Choose a reason for hiding this comment

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

if we use 1 in step, some AC with 0.5 step not worked correcly, and fahrenheit unit not worked too

Copy link
Contributor Author

@aabdellah aabdellah Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Anyway to detect the step from AC model json? I looked but I couldn't find it.
If not then it could be added in a map for each model.

As for Fahrenheit, step is still one my devices.

Copy link
Owner

Choose a reason for hiding this comment

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

Anyway to detect the step from AC model json? I looked but I couldn't find it.

I couldn't found anything too

If not then it could be added in a map for each model.

I tried this way before, but somehow, I got some device same model number but different step (as I remember)

As for Fahrenheit, step is still one my devices.

in Home app, you can adjust fahrenheit temperature in step 1 by 1, but Homekit always handle it in celcius, it's mean they will convert fahrenheit value to celcius and send it to server

btw, I have config for AC ac_temperature_unit, we can use this config value to change this step value I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer if I remove the stepping change for now and proceed with the PR and create a dedicated PR for step changes?
It seems that it will require a better look to make sure it's compatible with wide range of devices and to make sure that temp conversion handling is correct.

Copy link
Owner

Choose a reason for hiding this comment

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

I was mistaken about the Fahrenheit unit, a step of 1 or 0.01 works perfectly with Fahrenheit, but not with Celsius, for example people can't set temperature to 23.5 *C even their AC supported that range
so yes, we need revert it to step 0.01 to make sure it's compatible

what problem do you have about that step? we can discuss more
I tried to change temperature display unit in Home app to Fahrenheit and everything still look good even with step 0.01

};

if (minRange && maxRange) {
Expand Down Expand Up @@ -691,6 +693,7 @@ export default class AirConditioner extends baseDevice {
const targetHeatTemperature = targetTemperature(tempHeatMinRange, tempHeatMaxRange);

if (targetHeatTemperature) {
console.debug('targetHeatTemperature: ' + targetHeatTemperature.step);
this.service.getCharacteristic(Characteristic.HeatingThresholdTemperature)
.setProps({
minValue: this.Status.convertTemperatureCelsiusFromLGToHomekit(targetHeatTemperature.min),
Expand All @@ -705,6 +708,7 @@ export default class AirConditioner extends baseDevice {
const targetCoolTemperature = targetTemperature(tempCoolMinRange, tempCoolMaxRange);

if (targetCoolTemperature) {
console.debug('targetCoolTemperature: ' + targetCoolTemperature.step);
this.service.getCharacteristic(Characteristic.CoolingThresholdTemperature)
.setProps({
minValue: this.Status.convertTemperatureCelsiusFromLGToHomekit(targetCoolTemperature.min),
Expand Down
4 changes: 2 additions & 2 deletions src/lib/API.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,8 @@ export class API {
return this._homes;
}

public async sendCommandToDevice(device_id: string, values: Record<string, any>, command: 'Set' | 'Operation', ctrlKey = 'basicCtrl') {
return await this.postRequest('service/devices/' + device_id + '/control-sync', {
public async sendCommandToDevice(device_id: string, values: Record<string, any>, command: 'Set' | 'Operation', ctrlKey = 'basicCtrl', ctrlPath = 'control-sync') {
return await this.postRequest('service/devices/' + device_id + '/' + ctrlPath, {
ctrlKey,
'command': command,
...values,
Expand Down
4 changes: 2 additions & 2 deletions src/lib/ThinQ.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ export class ThinQ {
});
}

public deviceControl(device: string | Device, values: Record<string, any>, command: 'Set' | 'Operation' = 'Set', ctrlKey = 'basicCtrl') {
public deviceControl(device: string | Device, values: Record<string, any>, command: 'Set' | 'Operation' = 'Set', ctrlKey = 'basicCtrl', ctrlPath = 'control-sync') {
const id = device instanceof Device ? device.id : device;
return this.api.sendCommandToDevice(id, values, command, ctrlKey)
return this.api.sendCommandToDevice(id, values, command, ctrlKey, ctrlPath)
.then(response => {
if (response.resultCode === '0000') {
this.log.debug('ThinQ Device Received the Command');
Expand Down