search

sm6yvr / liam

DIY Robot lawn mover
GNU General Public License v3.0
52 stars 23 forks source link

A bit of code cleanup #32

Closed thomasloven closed 6 years ago

thomasloven commented 6 years ago

A number of minor changes.

Mostly removing unused code and variables. Commenting a few things I found hard to understand from the code at first glance.

One big thing; I changed all references to "SOC" to say "Voltage", because that's what it is. See further comment in the "Cleaning up Battery library" commit message.

sm6yvr commented 6 years ago

Hi and thanks for the PR and your work! After a fast review I found that you have removed

We are using them in the new mqtt-version so could you please add them and send a new PR? BR

thomasloven commented 6 years ago

Sure thing!

My thought was that they could be reimplemented later, when needed, but I guess I was a bit too strong on the YAGNI. I'll fix it when the kids are asleep.

thomasloven commented 6 years ago

There. I also added two commits with some cleanup in the display and error libraries. Some of those changes clash with what @Ola-Palm has in his develop branch. Perhaps he should be allowed to comment before this is pulled in...

sm6yvr commented 6 years ago

I have asked @Ola-Palm to review your changes

thomasloven commented 6 years ago

Oh, and also, I don't have an LCD, so I can't test if my changes to myLCD actually work as intended...

Ola-Palm commented 6 years ago

I see no issues here. We have dug i bit in the same part of the code, but the changes I've made in my develop is at this moment more of -"hmm.. let's try this" and not with a fixed goal in mind. I need to add the API part to this but all that needs to be reworked now when we have better knowledge on how we shall do that implementation anyway. So no worries from my side.

nicly done.