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.

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

Drop support for connection to Senso on port 55568 #128

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

krksgbr
Copy link
Contributor

@krksgbr krksgbr commented Mar 28, 2024

Referring to a previous discussion we had:

Sensos with FW this old should be paired with an outdated PC, not PlayOS, so would not get a driver update we release through PlayOS.

This removes a lot of noise from the logs.

Checklist

  • [ ] Changelog updated
  • [ ] Code documented

Trying to connect to this port adds a lot of noise in the logs. Sensos with FW this old should be paired with an outdated PC, not PlayOS, so would not get a driver update we release through PlayOS.
@krksgbr krksgbr requested a review from knuton March 28, 2024 17:44
@krksgbr krksgbr added the reviewable Ready for initial or iterative review. label Mar 28, 2024
@knuton
Copy link
Member

knuton commented Apr 12, 2024

FYI, this looks good, but I still want to think deeply through whether this could reach any device combination that would break.

@knuton
Copy link
Member

knuton commented Apr 30, 2024

I am not confident this is safe to ship. The chance that this would reach an incompatibly outdated Senso is small but not zero. If it did, this might create a logistic hassle.

I see the following options (combineren mogelijk):

  • Add telemetry to try to catch any 1.2.0.0 devices in the wild
  • Move ahead and break things, trusting that any device reached by this change would be eligible for firmware upgrade to fix the issue (we would need to test this path)
  • Add a parameter to connectTCP to define whether to give up after a while
  • Add a parameter to connectTCP to define whether to log failure (or at which level)

@knuton knuton added details needed Further information requested to better evaluate changes and removed reviewable Ready for initial or iterative review. labels Apr 30, 2024
@knuton
Copy link
Member

knuton commented May 1, 2024

Something like this maybe?

diff --git a/src/dividat-driver/senso/main.go b/src/dividat-driver/senso/main.go
index 2b0ce4f..cd8e3a3 100644
--- a/src/dividat-driver/senso/main.go
+++ b/src/dividat-driver/senso/main.go
@@ -72,9 +72,12 @@ func (handle *Handle) Connect(address string) {
 		handle.broker.TryPub(data, "rx")
 	}
 
-	go connectTCP(ctx, handle.log.WithField("channel", "data"), address+":55568", handle.broker.Sub("noTx"), onReceive)
+	// Attempt to connect to Senso at port 55568 for legacy firmware versions <= 1.2.0.0
+	// Reduce to DEBUG log level for this connection loop, as failure to connect is expected for most devices now
+	go connectTCP(ctx, handle.log.WithField("channel", "data"), logrus.DebugLevel, address+":55568", handle.broker.Sub("noTx"), onReceive)
 	time.Sleep(1000 * time.Millisecond)
-	go connectTCP(ctx, handle.log.WithField("channel", "control"), address+":55567", handle.broker.Sub("tx"), onReceive)
+	// Connect to Senso at the port used by both current and legacy firmware versions
+	go connectTCP(ctx, handle.log.WithField("channel", "control"), logrus.InfoLevel, address+":55567", handle.broker.Sub("tx"), onReceive)
 
 	handle.cancelCurrentConnection = cancel
 }
diff --git a/src/dividat-driver/senso/tcp.go b/src/dividat-driver/senso/tcp.go
index 6794063..d915105 100644
--- a/src/dividat-driver/senso/tcp.go
+++ b/src/dividat-driver/senso/tcp.go
@@ -20,7 +20,7 @@ const maxInterval = 30 * time.Second
 type onReceive = func([]byte)
 
 // connectTCP creates a persistent tcp connection to address
-func connectTCP(ctx context.Context, baseLogger *logrus.Entry, address string, tx chan interface{}, onReceive onReceive) {
+func connectTCP(ctx context.Context, baseLogger *logrus.Entry, logLevel logrus.Level, address string, tx chan interface{}, onReceive onReceive) {
 	var dialer net.Dialer
 
 	var log = baseLogger.WithField("address", address)
@@ -34,11 +34,11 @@ func connectTCP(ctx context.Context, baseLogger *logrus.Entry, address string, t
 			conn.Close()
 		}
 
-		log.Info("Dialing TCP connection.")
+		logAt(logLevel, log, "Dialing TCP connection.")
 		conn, connErr = dialer.DialContext(ctx, "tcp", address)
 
 		if connErr != nil {
-			log.WithError(connErr).Info("Could not connect with Senso.")
+			logAt(logLevel, log.WithError(connErr), "Could not connect with Senso.")
 		}
 		return connErr
 	}
@@ -120,7 +120,6 @@ func tcpReader(log *logrus.Entry, conn net.Conn, channel chan<- []byte) {
 			} else if err, ok := readErr.(net.Error); ok && err.Timeout() {
 				// Read timeout, just continue Nothing
 			} else {
-				// log.WithError(readErr).Error("Read error.")
 				return
 			}
 		} else {
@@ -129,6 +128,27 @@ func tcpReader(log *logrus.Entry, conn net.Conn, channel chan<- []byte) {
 	}
 }
 
+func logAt(level logrus.Level, logger *logrus.Entry, msg string) {
+	switch level {
+	case logrus.TraceLevel:
+		logger.Trace(msg)
+	case logrus.DebugLevel:
+		logger.Debug(msg)
+	case logrus.InfoLevel:
+		logger.Info(msg)
+	case logrus.WarnLevel:
+		logger.Warn(msg)
+	case logrus.ErrorLevel:
+		logger.Error(msg)
+	case logrus.FatalLevel:
+		logger.Fatal(msg)
+	case logrus.PanicLevel:
+		logger.Panic(msg)
+	default:
+		logger.Info(msg)
+	}
+}
+
 func write(conn net.Conn, data []byte) error {
 	if conn != nil {
 		conn.SetWriteDeadline(time.Now().Add(1 * time.Millisecond))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
details needed Further information requested to better evaluate changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants