Skip to content

Stability fixes for discovery and reconnection #83

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

Open
ttherbrink wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Stability fixes for discovery and reconnection #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions lib/KNXInterface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
this.knxEventCaptures = [];

// Setup logging and binding for fucntions
this.log = console.log.bind(this, (`[Interface: ${this.name}]`));

Check warning on line 31 in lib/KNXInterface.js

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected console statement

// Object arrays with callbacks
this.onKNXConnectionCallbacks = [];
Expand All @@ -41,7 +41,7 @@
// Function to create the KNX connection
createConnection() {
// The KNX tunnel connection itself through the knx library
if (this.isConnected === true) return; // Skip re-initializing when already connected.
if (this.isConnected === true && !this.isTimedOut) return; // Skip re-initializing when already connected and healthy.
try {
this.knxConnection = new knx.Connection({
ipAddr: this.ipAddress, // IP address obtained through interfacemanager
Expand Down Expand Up @@ -71,6 +71,7 @@
disconnected: () => {
this.log('KNX Disconnected');
this.isConnected = false;
this.isTimedOut = false;

this.onKNXConnectionCallbacks.forEach((callback) => {
try {
Expand Down Expand Up @@ -125,6 +126,7 @@

// Safely disconnect and reconnect the KNX connection
_reconnect() {
this.isTimedOut = false;
if (!this.knxConnection) {
this.isConnected = false;
this.createConnection();
Expand Down Expand Up @@ -235,9 +237,11 @@
return this.knxCommunicationQueue.add(async () => {
// Only add the request in the queue when the connection is openend.
return new Promise((resolve, reject) => {
this.knxConnection.read(groupaddress, (src, data) => resolve(data));
// Trigger a timeout, inherited from the library timeout (3s)
setTimeout(() => reject(new Error(`knx_read_timeout: ${groupaddress}`)), 3000);
const timeout = setTimeout(() => reject(new Error(`knx_read_timeout: ${groupaddress}`)), 3000);
this.knxConnection.read(groupaddress, (src, data) => {
clearTimeout(timeout);
resolve(data);
});
});
});
}
Expand Down
36 changes: 19 additions & 17 deletions lib/KNXInterfaceManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
const KNXInterface = require('./KNXInterface');

// 3 minute search loop
// const FIND_DEVICES_INTERVAL = 3 * (60 * 1000);
const FIND_DEVICES_INTERVAL = 3 * (60 * 1000);

class KNXInterfaceManager extends EventEmitter {

Expand All @@ -16,8 +16,8 @@
this.homey = homey;

// Formatted logging
this.log = console.log.bind(this, '[KNX interface manager]');

Check warning on line 19 in lib/KNXInterfaceManager.js

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected console statement
this.errorLog = console.error.bind(this, '[KNX interface manager] ERROR:');

Check warning on line 20 in lib/KNXInterfaceManager.js

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected console statement

// search running status
this.searchRunning = false;
Expand All @@ -25,7 +25,7 @@
this.KNXInterfaces = {};

if (ip.isV4Format(localIP)) {
this.localIPBuffer = ip.toBuffer(ip.address());
this.localIPBuffer = ip.toBuffer(localIP);
const interfaces = this.homey.settings.get('interfaces') || [];

this.findKNXInterfaces()
Expand All @@ -48,12 +48,12 @@
return;
}

// this.findDevicesInterval = setInterval(() => {
// this.findKNXInterfaces()
// .catch(error => {
// this.log('findKNXinterfaces error', error);
// });
// }, FIND_DEVICES_INTERVAL);
this.findDevicesInterval = setInterval(() => {
this.findKNXInterfaces()
.catch((error) => {
this.log('findKNXinterfaces error', error);
});
}, FIND_DEVICES_INTERVAL);
Comment on lines +51 to +56
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

The periodic discovery interval is started in the constructor but there’s no corresponding cleanup (e.g., clearInterval) when the app stops or if a new KNXInterfaceManager instance is created. This can leave multiple discovery loops running and keep resources alive longer than intended. Consider adding a destroy()/onUninit-style cleanup that clears this.findDevicesInterval (and have the app call it on shutdown/reload).

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +56
Copy link
Copy Markdown
Contributor

@wouter-athom wouter-athom Apr 14, 2026

Choose a reason for hiding this comment

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

The fact that this was here already but commented out makes me wonder why. Was there a good reason for not doing it like this?

Copy link
Copy Markdown
Contributor Author

@ttherbrink ttherbrink Apr 14, 2026

Choose a reason for hiding this comment

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

Could not find a good reason. It was disabled over 5 years ago so no way to know


// Respond on an emit. Add the found interface to the list of interfaces.
this.on('interface_found', (knxInterface) => {
Expand Down Expand Up @@ -207,7 +207,7 @@
udpSocket
.on('error', (err) => {
// If the udp server errors, close the connection
console.error('udp server error:', err.stack);

Check warning on line 210 in lib/KNXInterfaceManager.js

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected console statement
try {
udpSocket.close();
} catch (e) {
Expand All @@ -220,10 +220,9 @@
// When an message is received, parse it for KNXIP data
const knxInterface = this.parseKNXResponse(msg);
if (!knxInterface) {
return reject(new Error('No valid KNXnet response'));
return null; // Skip non-KNX messages, keep listening for valid responses
}
Comment on lines 221 to 224
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

parseKNXResponse() logs for every non-KNX UDP packet, and the new behavior here keeps listening instead of failing fast. On noisy networks this can turn into high-volume log spam during each discovery window. Consider silencing this path (or gating it behind a debug flag / rate limiting) when returning null for non-KNX messages.

Copilot uses AI. Check for mistakes.
// If the data was valid, emit the found interface
// here should be an extra catch to handle other not search/check related KNX messages
try {
if (this.KNXInterfaces[knxInterface.interfaceMac]) {
this.log('Updating', knxInterface.interfaceName);
Expand All @@ -238,10 +237,8 @@
this.homey.settings.set('interfaces', interfaces);

this.emit('interface_found', this.KNXInterfaces[knxInterface.interfaceMac]);
return resolve(this.KNXInterfaces[knxInterface.interfaceMac]);
} catch (error) {
this.log('Creating IP interface instance failed: ', error);
reject(error);
}
return null;
})
Expand Down Expand Up @@ -288,7 +285,6 @@
- Send the description request to obtain the device information
These actions mimics the traffic that ETS uses to check a IP interface
*/
this.interfaceFound = false;
if (this.searchRunning === false) {
this.log('Checking if', ipAddress, 'is a KNX IP interface');
this.searchRunning = true;
Expand Down Expand Up @@ -319,10 +315,12 @@
const udpSocket = dgram.createSocket('udp4'); // Create the socket connections

return new Promise((resolve, reject) => {
let interfaceFound = false;

udpSocket
.on('error', (err) => {
// If the udp server errors, close the connection
Comment on lines 315 to 322
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

Inside this Promise, the UDP message handler currently does if (!commResult) return new Error('No valid KNX message parsed');. Returning an Error object from an event handler doesn’t propagate or reject the Promise, so it’s misleading and wastes work. Prefer return null/return (or explicitly reject(...) if you want the check to fail fast) so the control flow is correct and clear.

Copilot uses AI. Check for mistakes.
console.error('UDP server error', err.stack);

Check warning on line 323 in lib/KNXInterfaceManager.js

View workflow job for this annotation

GitHub Actions / Lint 

Unexpected console statement
try {
udpSocket.close();
} catch (e) {
Expand Down Expand Up @@ -361,12 +359,16 @@
} catch (e) {
// socket may already be closed
}
this.interfaceFound = true;
interfaceFound = true;
this.searchRunning = false;

Comment on lines 359 to 364
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

The udpSocket.send(..., (err) => { if (err) throw err; }) path (in this same Promise) can crash the process because the exception is thrown from an async callback. Handle it like other error paths: close the socket, reset this.searchRunning, and reject(err) instead of throwing.

Copilot uses AI. Check for mistakes.
try {
// Use the Interface MAC as the key value
this.KNXInterfaces[knxInterface.interfaceMac] = new KNXInterface(knxInterface);
// Update existing interface or create a new one
if (this.KNXInterfaces[knxInterface.interfaceMac]) {
this.KNXInterfaces[knxInterface.interfaceMac].updateIP(knxInterface.interfaceIp);
} else {
this.KNXInterfaces[knxInterface.interfaceMac] = new KNXInterface(knxInterface);
}
const interfaces = this.getSimpleInterfaceList();
this.homey.settings.set('interfaces', interfaces);
this.emit('interface_found', this.KNXInterfaces[knxInterface.interfaceMac]);
Expand All @@ -389,7 +391,7 @@
.bind(36711);

setTimeout(() => {
if (!this.interfaceFound) {
if (!interfaceFound) {
try {
udpSocket.close();
} catch (error) {
Comment on lines 391 to 397
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

If an error occurs while sending the initial connect request (in the listening handler), the Promise is rejected but this.searchRunning may remain true until the timeout runs. Consider resetting this.searchRunning and closing the socket immediately on send failure to avoid temporarily blocking other discovery/check calls.

Copilot uses AI. Check for mistakes.
Expand Down
Loading