-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
mqtt: add support for ColorSettingTemperature
and ColorSettingHsv
#1317
Conversation
plugins/mqtt/package.json
Outdated
"aedes": "^0.46.1", | ||
"axios": "^0.23.0", | ||
"axios": "^1.6.7", |
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.
this version of axios adds a 4MB dependency on mime db. it's why I haven't upgraded. I'm actually migrating off axios and using fetch directly since that is in node 18.
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.
reverted
plugins/mqtt/package.json
Outdated
"aedes": "^0.46.1", | ||
"axios": "^0.23.0", | ||
"axios": "^1.6.7", | ||
"color-convert": "^2.0.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.
I'd rather not introduce a dependency for a simple function here: from copilot:
function rgbToHsv(r: number, g: number, b: number): [number, number, number] {
r /= 255, g /= 255, b /= 255;
let max = Math.max(r, g, b), min = Math.min(r, g, b);
let h, s, v = max;
let d = max - min;
s = max == 0 ? 0 : d / max;
if (max == min) {
h = 0; // achromatic
} else {
switch (max) {
case r: h = (g - b) / d + (g < b ? 6 : 0); break;
case g: h = (b - r) / d + 2; break;
case b: h = (r - g) / d + 4; break;
}
h /= 6;
}
return [h, s, v];
}
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.
this dependency looks relatively small and 1 level deep, so this could be fine. I generally prefer avoiding that on trivial stuff like this.
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 have completed this item
plugins/mqtt/src/util.ts
Outdated
@@ -0,0 +1,227 @@ | |||
import convert from 'color-convert' |
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 think color-util.ts would be a better filename
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.
done
I will downgrade it and resubmit. Sent from my iPhone, expect autocorrection issuesOn Feb 13, 2024, at 5:49 PM, Koushik Dutta ***@***.***> wrote:
@koush commented on this pull request.
In plugins/mqtt/package.json:
"aedes": "^0.46.1",
- "axios": "^0.23.0",
+ "axios": "^1.6.7",
this version of axios adds a 4MB dependency on mime db. it's why I haven't upgraded. I'm actually migrating off axios and using fetch directly since that is in node 18.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
I was having the same debate with myself. I will remove and include
directly in ‘util.js’
…On Tue, Feb 13, 2024 at 5:51 PM Koushik Dutta ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In plugins/mqtt/package.json
<#1317 (comment)>:
> "aedes": "^0.46.1",
- "axios": "^0.23.0",
+ "axios": "^1.6.7",
+ "color-convert": "^2.0.1",
I'd rather not introduce a dependency for a simple function here: from
copilot:
function rgbToHsv(r: number, g: number, b: number): [number, number, number] {
r /= 255, g /= 255, b /= 255;
let max = Math.max(r, g, b), min = Math.min(r, g, b);
let h, s, v = max;
let d = max - min;
s = max == 0 ? 0 : d / max;
if (max == min) {
h = 0; // achromatic
} else {
switch (max) {
case r: h = (g - b) / d + (g < b ? 6 : 0); break;
case g: h = (b - r) / d + 2; break;
case b: h = (r - g) / d + 4; break;
}
h /= 6;
}
return [h, s, v];}
—
Reply to this email directly, view it on GitHub
<#1317 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACEH5HH37W65V7F3KX76G3YTPU5ZAVCNFSM6AAAAABDHJJGO2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZZGA4DKMJRHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
7ff232e
to
cffca24
Compare
I have addressed the above requests, and squashed them back into the commit |
@@ -1,6 +1,6 @@ | |||
{ | |||
"compilerOptions": { | |||
"module": "commonjs", | |||
"module": "Node16", |
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 dont think this is correct, you probably want this?
"module": "CommonJS",
"moduleResolution": "Node16",
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 will revert. It was likely troubleshooting that didn’t yield any change.
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 just noticed that my tsconfig is also complaining to change this, so I'm not sure what this is about. wasn't doing that before. might have been a node typings change. I'll investigate further later.
…o the MQTT support. (koush#1317)
I have added support for colored lights in the current MQTT design. In my testing this works with both Z2M and ZHA in Home Assistant.