I made some small upgrades to the amazing library you built#1
Open
nitaybz wants to merge 63 commits into
Open
Conversation
added 8 commits
September 24, 2020 11:11
Owner
|
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
Owner
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) |
Owner
There was a problem hiding this comment.
Have you tested all three devices?
| 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.`); |
| SWITCHER_PORT = 9957; | ||
|
|
||
| constructor(device_id, switcher_ip, phone_id, device_pass, log) { | ||
| constructor(device_id, switcher_ip, log, listen) { |
Owner
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
| this.p_session = null; | ||
| this.socket = null; | ||
| this.status_socket = this._hijack_status_report(); | ||
| if (listen) |
Owner
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
| 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) { |
Owner
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?
| return proxy; | ||
| } | ||
|
|
||
| static listen(log, identifier) { |
Owner
There was a problem hiding this comment.
can you please elaborate why this listen method is needed?
| name: device_name, | ||
| state: state, | ||
| device_id: this.device_id, | ||
| power: state, |
Owner
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
| 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); |
Owner
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.
| var device_id = udp_message.extract_device_id() | ||
| if (device_id === this.device_id) | ||
| this.emit(STATUS_EVENT, { | ||
| device_id: device_id, |
| ConnectionError: ConnectionError, | ||
| ON: ON, | ||
| OFF, OFF | ||
| ConnectionError: ConnectionError |
Owner
There was a problem hiding this comment.
Why did you remove the ON and OFF?
- fix this.log → log in static discover() / listen() (this.log is undefined in static method context; use the log parameter instead) - fix accidental semicolon on `if (discovery_timeout);` so the discovery timeout actually respects the parameter instead of always firing - correct close log message in listen() (was 'closing discover socket') - bump runtime deps: adm-zip ^0.5.17, python-struct ^1.1.3 - bump devDep eslint to ^9.39.4 and declare engines.node >=18 Closes #2 #3 Credit to @mbravorus for the static method fix (#3) and @LeonFedotov for spotting the semicolon (#2).
The OnWall sends device-type byte 0321 in its UDP broadcast, which had no entry in the types map and showed up as UNKNOWN_0321. It's an old-protocol boiler with the same state shape as v3/v4/mini, so: - udp.js: map 0321 → 'on_wall' - switcher.js: include 'on_wall' in OLD_TCP_GROUP and in the boiler state-emit branch of listen() Closes nitaybz/homebridge-switcher-platform#67 nitaybz/homebridge-switcher-platform#73
- move on_wall out of OLD_TCP_GROUP so it tries the new TCP port (10000) instead of 9957, which an OnWall device reported as ECONNREFUSED. We do not actually have an OnWall device to verify but this is the next reasonable port to try. - wrap _run_power_command, _run_general_command, and set_default_shutdown in try/catch that emits ERROR_EVENT instead of letting connection errors propagate as unhandled rejections. Previously a single device's TCP failure could crash the entire Homebridge child bridge.
…ures After a long idle period (overnight, WiFi reassoc, device firmware hiccups), the cached TCP socket to a Switcher device could go stale without Node noticing. The next command would then fail with ECONNRESET on login and the user had to restart Homebridge to recover. Three changes to fix that: - _getsocket: when the cached socket emits error or close, null out this.socket and this.p_session immediately so the next command opens a fresh connection instead of writing to a dead one. - _connect: setKeepAlive(true, 30000) so dead idle connections are detected within ~30s instead of the OS default ~2 hours. - _run_power_command and _run_general_command: wrap the body in an inner attempt() and retry once on failure after _reset_connection_state. This handles the very first command after idle without bothering the user. Closes nitaybz/homebridge-switcher-platform#55
- log device_type, newType flag, and resolved TCP port at construction so we can see at startup whether the protocol path is correct - log opening connection and ready event with ip:port instead of a generic 'successful connection' message, so we can confirm whether the right port is being used - tag each login flow distinctly (_login old-TCP / _login2 new-TCP / _login3 token-auth) instead of three identical 'login...' lines - log 'waiting for response' after every login and command write so silent device timeouts are visible in the log instead of just trailing off into nothing No behavior change, debug output only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hope you'll merge :)