search

linuxkidd / coachproxy-os

Open Source version of CoachProxy software
GNU General Public License v3.0
36 stars 19 forks source link

Remove unnecessary JSON nodes #14

Open cterwilliger opened 4 years ago

cterwilliger commented 4 years ago

Remove JSON nodes required by early version of node-red and configure their upstream nodes to provide JSON output. Do we expect this will improve performance or only simplify the flow (which is still a bonus). I'll volunteer to do it...

greendog99 commented 4 years ago

I think it just simplifies the flow.

greendog99 commented 4 years ago

Thinking about this a bit more, and since this change adds no tangible benefit, I wonder if it makes more sense to start from scratch on the flows to see what other optimizations can be made... my new Canbus decoder provides an easier way to get at some of the RVC data, which could also simplify things.

cterwilliger commented 4 years ago

Sure, that makes sense. I looked at your new code, it does look simpler and cleaner than the old perl code, but now I have to come up to speed on Ruby... So, are you running a separate RPi with the new code to build and test? Wondering if I can start substituting Ruby for Perl modules on the same RPI.

greendog99 commented 4 years ago

I am running a separate RPi for development efforts, but you could easily run both on the same RPi. I think Python would be ideal as it's more widely used than Ruby, but I'm not nearly as proficient in it so I went with Ruby. Feel free to throw out other ideas... I just really wanted to ditch Perl :-)

cterwilliger commented 4 years ago

I've got several changes & enhancements, but don't know if you want to throw those in to legacy while working on the Ruby platform. 1. I needed to merge my batteries on command, so I created that. 2. I don't like Temps displayed in tenths, there's no way the sensors are that good anyway, so I removed that. 3. I implemented Pushover where you left a blank space for it. 4. Added inverter temperatures to monitoring after one of my neighbors experienced bad fans. 5. added battery bay temp

greendog99 commented 4 years ago

I also set up a battery merge for my personal use.

I think 1, 4, and 5 are probably specific to certain coach models and years. Do you have it set up so those only show up in the UI by request or by year/model? If not, I could help get that part done since that code can be tricky. Knowing what features exist on each model and year has been one of the hardest things to figure out though. For 2, I like the tenths so I can at least see if/when the thermostat might be ready to turn on/off and if the temp reading is actually changing (e.g. on really hot days when the AC is having a hard time keeping up). We could make it a configurable option. For 3, we could include Pushover for everyone along with a little text explaining what it is. I originally took it out since I figured it wouldn't interest 99% of users and had the potential to confuse people.

As for whether or not to include these changes in legacy or not, my preference is to try to forge forward with something new (and hopefully easier to modify / maintain) unless some of these would have a big impact/win for existing users.