I made some small upgrades to the amazing library you built#1
I made some small upgrades to the amazing library you built#1nitaybz wants to merge 57 commits intojohnathanvidu:masterfrom
Conversation
|
Thank you for taking the time to contribute to switcher-js. I'll take a look at your changes in the coming days |
johnathanvidu
left a comment
johnathanvidu
left a comment
There was a problem hiding this comment.
thanks again for contributing! please refer to my comments
| - **Switcher V3** (Switcher touch) - FW **V1.51** | ||
| - **Switcher V3**: (Switcher touch) - Firmware **V1.51** | ||
| - **Switcher V2**: Firmware **3.21** (Based on ESP chipset) | ||
| - **Switcher V2**: Firmware**72.32** (Qualcomm chipset) |
There was a problem hiding this comment.
Have you tested all three devices?
src/switcher.js
Outdated
| class ConnectionError extends Error { | ||
| constructor(ip, port) { | ||
| super('connection error: failed to connect to switcher on ip: ${ip}:${port}. please make sure it is turned on and available.'); | ||
| super(`connection error: failed to connect to switcher on ip: ${ip}:${port}. please make sure it is turned on and available.`); |
src/switcher.js
Outdated
| SWITCHER_PORT = 9957; | ||
|
|
||
| constructor(device_id, switcher_ip, phone_id, device_pass, log) { | ||
| constructor(device_id, switcher_ip, log, listen) { |
There was a problem hiding this comment.
are you sure phone_id and device_pass aren't required for older firmware? cause as far as I understand they are
src/switcher.js
Outdated
| this.p_session = null; | ||
| this.socket = null; | ||
| this.status_socket = this._hijack_status_report(); | ||
| if (listen) |
There was a problem hiding this comment.
I would prefer to always listen to status messages. I want the user to always be able to rely on proxy.on('status') to work, no matter how to switcher class was configured
src/switcher.js
Outdated
| var device_id = udp_message.extract_device_id(); | ||
| proxy.emit(READY_EVENT, new Switcher(device_id, ipaddr, phone_id, device_pass, log)); | ||
| var device_name = udp_message.extract_device_name(); | ||
| if (identifier && identifier !== device_id && identifier !== device_name && identifier !== ipaddr) { |
There was a problem hiding this comment.
I like the discovery timeout, but can you elaborate on the identifier? I don't really understand why we need it. Are you assuming a house with multiple switchers?
src/switcher.js
Outdated
| return proxy; | ||
| } | ||
|
|
||
| static listen(log, identifier) { |
There was a problem hiding this comment.
can you please elaborate why this listen method is needed?
src/switcher.js
Outdated
| name: device_name, | ||
| state: state, | ||
| device_id: this.device_id, | ||
| power: state, |
There was a problem hiding this comment.
why did you remove name? and state can be changed to power, though I prefer something like power_state, but it is a breaking change, so maybe we can change it in a different commit, and increase the minor/major version
src/switcher.js
Outdated
| var socket = await this._connect(this.SWITCHER_PORT, this.switcher_ip); | ||
| socket.on('error', (error) => { | ||
| this.log.debug('gloabal error event:', error); | ||
| this.log('gloabal error event:', error); |
There was a problem hiding this comment.
the log function is basically a logger (I used the one I receive from homebridge) so your changes will break the homebridge plugin.
src/switcher.js
Outdated
| var device_id = udp_message.extract_device_id() | ||
| if (device_id === this.device_id) | ||
| this.emit(STATUS_EVENT, { | ||
| device_id: device_id, |
src/switcher.js
Outdated
| ConnectionError: ConnectionError, | ||
| ON: ON, | ||
| OFF, OFF | ||
| ConnectionError: ConnectionError |
There was a problem hiding this comment.
Why did you remove the ON and OFF?
2 participants
Hope you'll merge :)