Minor code cleanup + add warning in documentation about flat top vs button top batteries

nplan / HomeButtons

140 stars 9 forks source link

Minor code cleanup + add warning in documentation about flat top vs button top batteries #19

Closed mcarbonne closed 1 year ago

mcarbonne commented 1 year ago

I received my device today and encountered some minor issues:

"button top" 18650 batteries does not to fit at all => I updated specs and and setup guide
I was having some issues so I started to dig into the code. Mostly network isn't reliable at all and response time is random (from 2 seconds to maybe up to 10 seconds, but ~5s most of the time). And sometimes, I got a network error message. I haven't found yet what is causing me trouble but I did some (light) code clean-up (split in several commits with explicit messages for an easy review).

I tested this version on my device and it seems to work, at least as good as the previous version (2.0.2).

I plan to do more PR until I fix my issues. Feel free to comment my changes.

nplan commented 1 year ago

How high are your button top cells? As far as I know most of them are around 66.5 mm which should still fit... Maybe it would be better to specify max height.

nplan commented 1 year ago

What is the reason you changed all the include guards from UUID to PROJECT_FILENAME?

nplan commented 1 year ago

Regarding the network reliability: I just pushed a fix to dev branch which should help if you have multiple APs in your network.

mcarbonne commented 1 year ago

My button top cells are 67mm high. But indeed, it does not seem standard at all. So I'll edit my warning to something like Warning: some button top cells might not fit (maximum length: XX.XX mm). What is the maximum size supported by the holder ? 66.5 mm ? or 65.0 mm ? (My 65mm flat top cells seems already very tight).

Concerning UUID, there is no "good" reason except the following:

I did not guess it was UUID (as I never saw this before)
I did not find any explanation about it (generation rules if I create a new header file for example ?).
It is not very human friendly

If you prefer to keep UUID, it's ok for me, but I would rather add a codingrules.md in Firmware folder giving a brief explanation (and maybe how to setup IDE to automatically generate them ?)

I'll have a look at your fixes concerning network reliability, thanks !

nplan commented 1 year ago

Checked a number of holders again and while there is some variability, 65.0 mm seems to be the maximum that would work in all cases.

I'll review the changes again and merge it. But I'll merge it to dev. master should be kept for releases only.

mcarbonne commented 1 year ago

I updated my PR for dev.

Concerning network issues, even with your last changes on dev, I keep having issues. I did a little benchmark by measuring time for a "press" to be taken into account, 5 times:

3 out of 5 => network issue message after ~45s (timeout related ?)
1 => 5s
1 => 10s

Sometimes, it works well a few times in a row, but it seems random. I'll plug an UART probe to get logs from the debug connector tomorrow (I assume logs are redirect to an UART output ?). EDIT : or maybe logs are available over USB ?

nplan commented 1 year ago

45s is the timeout. You'll get the error message then. You can get logs on the UART, there's nothing on the USB. Please share them. Maybe it's best that you create a new issue for this.