Merged "Analog Sensor API", "ProgramView", "Flow Alert" and "Monitor and Control" to current (2.0!) by opensprinklershop · Pull Request #206 · OpenSprinkler/OpenSprinkler-App

opensprinklershop · 2025-04-07T22:43:51Z

Analog Sensor API + Program View

…Control" to current

…8118ade0

mellodev · 2025-04-08T03:15:55Z

linter is failing @opensprinklershop

mellodev · 2025-04-22T00:30:50Z

hello @opensprinklershop looks like there is a small linting issue that still needs to be resolved:

mellodev

Hello, this is a massive PR! Lots of comments on this one:

mellodev · 2025-04-22T00:47:03Z

 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 -->
-<html>
+<html data-appversion="2.4.175">


I wanted to double check on this attribute data-appversion. I don't see any related code to read this value, is it required? Additionally, a few months ago there were some changes to how the app versioning works. All app version references are updated automatically by the github workflows associated with builds. I'd suggest either we either remove this attribute if it's not needed, or update the workflows to set this value automatically.

Yes, you can remove that. It was used to query the current version number.

mellodev · 2025-04-22T00:50:09Z

 		<!-- Load OSApp modules -->
 		<script src="js/modules/about.js"></script>
 		<script src="js/modules/analog.js"></script>
+		<script src="js/modules/programview.js"></script>


I'd suggest renaming this import and the file itself to program-view.js to match the file naming convention used by the other multi-word modules.

mellodev · 2025-04-22T01:19:05Z

+		CURRENT_FW : "2.3.3(172)",
+		CURRENT_FW_ID : 231,
+		CURRENT_FW_MIN : 150,


Perhaps renaming these variables to REQUIRED_FW, REQUIRED_FW_ID and REQUIRED_FW_MIN would be more descriptive? Later on in checkFirmwareUpdate we see these variables are being used to verify a minimum supported fw version for specific features and suggest the user update to a specific supported firmware if needed.

mellodev · 2025-04-22T02:10:31Z

 	}
 };

 OSApp.Analog.checkAnalogSensorAvail = function() {
-	return OSApp.currentSession.controller.options && OSApp.currentSession.controller.options.feature === "ASB";
+	return OSApp.currentSession.controller.options && OSApp.currentSession.controller.options.feature.includes("ASB");


This function is called from several other places in the app. I think this needs an elvis operator added to prevent an exception when options.feature is undefined.

Suggested change

return OSApp.currentSession.controller.options && OSApp.currentSession.controller.options.feature.includes("ASB");

return OSApp.currentSession.controller.options && OSApp.currentSession.controller.options.feature?.includes("ASB");

Tested this branch locally and I'm seeing this on launch:

As soon as the extensions of my extended firmware have been included in the standard firmware, this can be removed

mellodev · 2025-04-22T02:26:37Z

+};
+
+OSApp.Analog.checkMonitorAlerts = function() {
+	if (!window.cordova || !window.cordova.plugins || !OSApp.Analog.monitors || (!OSApp.currentDevice.isAndroid && !OSApp.currentDevice.isiOS))


This looks like a really good candidate to be refactored out to it's own function. Looks like we duplicate this exact/similar logic in several functions in analog.js

mellodev · 2025-04-22T05:15:56Z

+/* global $, ApexCharts */
+
+/*!
+ * Analog Sensor API - GUI for OpenSprinkler App
+ * https://github.com/opensprinklershop/
+ * (c) 2023 OpenSprinklerShop
+ * Released under the MIT License
+ */
+


I believe this is the new dashboard component to display programs? Is there any documentation about this functionality that needs to be updated?

The end-user documentation is easy: Press the "Start" button to start the program ;-)

mellodev · 2025-04-22T05:21:52Z

+	OSApp.Firmware.sendToOS("/du?pw=", "json").then( function( status ) {
+		var popup = "<div data-role='popup' id='debugWU' class='ui-content ui-page-theme-a'>";


⚠️ Should this be gated with a checkOSVersion call? When I run this code locally and click on the 'System Diagnostics' link in the left menu, I am getting 404s for the /du endpoint via OTC. These failures mean the Systen Diagnostics' modal never appears for me.

yes, looks like checkOSVersion is missing here.

mellodev · 2025-04-22T05:23:24Z

+		if ( typeof OSApp.currentSession.controller.settings.otcs === "number"  || (status && status.hasOwnProperty("status")) ) {
+			popup += "<div class='debugWUHeading'>Integrations</div>" +
+				"<table class='debugWUTable'>";
+			if (typeof OSApp.currentSession.controller.settings.otcs === "number")
+				popup += "<tr><td>OpenThings Cloud</td><td>" + OSApp.SystemDiagnostics.resolveOTCStatus( OSApp.currentSession.controller.settings.otcs ) + "</td></tr>";
+			if (status.hasOwnProperty("freeBytes"))
+				popup += "<tr><td>Free Bytes</td><td>" + OSApp.SystemDiagnostics.format2(status.freeBytes/1024) + " KB</td></tr>";
+			if (status.hasOwnProperty("pingok"))
+				popup += "<tr><td>Ping check ok</td><td>" + status.pingok + "</td></tr>";
+			if (status.hasOwnProperty("mqtt"))
+				popup += "<tr><td>MQTT</td><td>" + (status.mqtt?"connected":"disconnected") + "</td></tr>";
+			if (status.hasOwnProperty("influxdb"))
+				popup += "<tr><td>influxdb</td><td>" + (status.influxdb ? "enabled" : "disabled") + "</td></tr>";
+			if (status.hasOwnProperty("ifttt"))
+				popup += "<tr><td>IFTTT</td><td>" + (status.ifttt?"enabled":"disabled") + "</td></tr>";
+			popup +="</table>";
+		}


Consider passing all these string literals through the language translation OSApp.Language._()

mellodev · 2025-04-22T05:25:06Z

+OSApp.Utils.escapeJSON2 = function( json ) {
+	return JSON.stringify( json ).replace(/#/g, "%23").replace(/=/g, "%3D").replace( /\{|\}/g, "" );
+};
+


Out of curiosity, why are we implementing this ourselves rather than using encodeURIComponent? This escapeJSON2 and it's sibling escapeJSON both feature very limited/incomplete sets of character replacements/transformations.

I think the problem was, that encodeURIComponent does too much, also escapes " and = and other elements that are needed by the json format.

mellodev · 2025-04-22T05:29:37Z

Out of curiosity why are we upgrading from v3.35.1 to v3.50.0? This jumps about 15 minor version releases they've made. If the goal is to update to the latest in the 3.x.x major version, it looks like v3.54.1 is the latest:

apexcharts/apexcharts.js@v3.54.1...v3.54.1

Yes latest version is ok

mellodev · 2025-04-22T22:28:43Z

I checked this PR out locally, made a branch from it and resolved almost every comment above (the ones with thumbs up reactopm). There were a couple I wasn't sure about (eyeballs reaction) so I left that untouched. See the updated branch PR here: #213

Does that mean we can close this one and use my #213 going forward?

salbahra · 2025-05-30T14:11:19Z

Closing in favor of #213

Stefan Schmaltz added 3 commits April 7, 2025 00:22

Merged "Analog Sensor API", ProgramView, Flow Alert and "Monitor and …

43afe53

…Control" to current

Analog Sensor API and Program View Enhancements

8118ade

Merge https://github.com/OpenSprinkler/OpenSprinkler-App into Branch_…

ce5ce9f

…8118ade0

opensprinklershop changed the title ~~Branch_8118ade0~~ Merged "Analog Sensor API", "ProgramView", "Flow Alert" and "Monitor and Control" to current (2.0!) Apr 7, 2025

Stefan Schmaltz and others added 2 commits April 8, 2025 22:42

Fixt eslint errors

1227d3c

updated apexcharts, added timeout, fixed programview

387104c

mellodev mentioned this pull request Apr 22, 2025

Merged "Analog Sensor API", ProgramView, Flow Alert and "Monitor and Control" to current #205

Closed

mellodev reviewed Apr 22, 2025

View reviewed changes

mellodev mentioned this pull request Apr 22, 2025

chore(analog): analog branch cleanup efforts #213

Open

salbahra closed this May 30, 2025

	return OSApp.currentSession.controller.options && OSApp.currentSession.controller.options.feature.includes("ASB");
	return OSApp.currentSession.controller.options && OSApp.currentSession.controller.options.feature?.includes("ASB");

		OSApp.Firmware.sendToOS("/du?pw=", "json").then( function( status ) {
		var popup = "<div data-role='popup' id='debugWU' class='ui-content ui-page-theme-a'>";

Conversation

opensprinklershop commented Apr 7, 2025

Uh oh!

mellodev commented Apr 8, 2025

Uh oh!

mellodev commented Apr 22, 2025

Uh oh!

mellodev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mellodev Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mellodev commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salbahra commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mellodev Apr 22, 2025 •

edited

Loading

mellodev commented Apr 22, 2025 •

edited

Loading