robotpy / pynetworktables2js

Forwards NetworkTables traffic to a web page, allowing you to write custom dashboards for your robot using HTML/Javascript
MIT License
46 stars 18 forks source link

Merging features from my RoboRIO-webdash #3

Closed computer-whisperer closed 9 years ago

computer-whisperer commented 9 years ago

Before this was released, I built something similar as the base for my RoboRIO-Webdash: https://github.com/computer-whisperer/RoboRIO-webdash . Now that this standard is here, I would like to transition to using it instead.

However, there are some useful features that I built into my base, but lack proper representation here; here are some examples:

I was wondering if you would be interested in me merging some of these features into this project?

virtuald commented 9 years ago

Sure, I would create separate branches for each so they can be reviewed separately.

I'm nervous about buffering. Typical NT users aren't transmitting enough data where buffering is necessary. In addition, for a UI actually used in competition, minimizing the latency from button press to robot doing something is a high priority. Why buffer?

aiohttp support sounds good to me.

What does item 3 mean? Currently, there definitely are problems when you use the wrong types.

What does item 4 mean? Do you mean NT server or websocket server? NetworkTables has a concept of sequence numbers and such that aren't exposed to the user, that are used for synchronization when a NT client disconnects/reconnects to a server. We'd want to be careful to work with that, and not against it.

computer-whisperer commented 9 years ago

My original reasoning for the buffering was to save bandwidth when, for example, the robot updates lots of sensor values at a high rate. I now see that this may not be optimal behavior in certain instances, maybe we could set the update rate to a configurable value?

What do you think would be the best way to implement the aiohttp support? Would it make sense to refactor the logic from handlers.py into a base.py, and then make a tornado_handlers.py and aiohttp_handlers.py?

In my webdash I handled the types server-side by casting the string sent by the client to whatever type already existed in NetworkTables. Here it would probably be better to add type-specific setters and getters (eg, getString, putNumber, getBoolean) to the javascript API, as well as having putValue check and cast values to whatever was already in NetworkTables.

On item 4 I meant for when the networktables2js server disconnects from the NT server. This is especially important this year, when you need to be able to configure your autonomous mode via your web-based dashboard, and the robot is taking forever to connect via mDNS.

virtuald commented 9 years ago

Your reasoning for buffering makes sense if you run the dashboard on the robot. However, I would argue that running it on the driver station is simpler. In particular, it allows you to open multiple HTML pages and use the same amount of bandwidth for them, and it allows the dashboard to be shown even when you're having mDNS issues. You also don't incur any bandwidth issues when loading your web content -- which shouldn't be an issue, but you never know what someone might do with this...

You can try a base.py to see if that makes sense... but I suspect the websocket protocol stuff is so simple, and tornado and aiohttp are so different, that I would just have completely separate implementations.

JSON can contain the basic types (string, int, boolean), and so I just send the contents of the JSON to NetworkTable.putValue on the python side, which detects the type appropriately and calls the right function. I strongly prefer this mechanism rather than sending a string over and trying to figure out the right type. On the HTML side when you call putValue, the caller is expected to send the proper Javascript type over. The side effect of this is that if you get the type wrong, then currently the errors only show up on the python side. Patches to make this behavior more sane are welcome.

I would want to be careful about any kind of disconnected operation stuff. One thing that we've ran into when reloading that gets really annoying is when the NetworkTables client (like SmartDashboard, or TableViewer) is still connected, so it has all of the old values and the robot uses those values instead of the values we just set in the code. The solution to that problem was just make sure that the robot always does a put on any variables that we later call 'get' on. If the dashboard is actively trying to maintain state... that could lead to hard to debug situations if not done right.

virtuald commented 9 years ago

Oh, and one more reason for running it on the driver station -- the websocket protocol is significantly easier to implement than NetworkTables, so we were able to write a C# interface very quickly and use that for some processing. Because it's just talking on the driver station, we get that without any additional bandwidth costs.

computer-whisperer commented 9 years ago

I guess that makes sense, though I would argue that there are still valid use-cases for running the server on the robot.

I will close this issue and open individual issues or PRs as needed,

virtuald commented 9 years ago

I agree that there are valid use cases for using it on the robot. One place where buffering may make sense is from the robot to the webpage. Here, you're often going to be displaying a value to the user, so it's probably ok if you wait 50ms before sending it to them.

Also NetworkTables does its own buffering internally... so there's already a 50ms latency built into the system (which I suppose you could use to argue that buffering on the client side isn't a significantly bad thing).