andreisperid / E-TKT

open source embossed label maker
https://andreisperid.github.io/E-TKT/
MIT License
398 stars 19 forks source link

Break out firmware into multiple classes. #46

Closed sabeechen closed 1 year ago

sabeechen commented 1 year ago

This PR breaks out the firmware into many different classes, based on functionality. Highlights are:

So, this PR is HUGE. I re-wrote literally all the C++. I've tested each function on my own and worked out all the little bugs I introduced (I think), but I'd appreciate if you could run this through its paces on your setup. I wasn't able to find anywhere this behaves differently than the original firmware, but its hard to know for sure.

If you have any feedback on the divisions of responsibility, file structure, anything, I'd be happy to integrate it. I'm no married to any decision, these just seemed like the best choices to make based on my experience, so its all subjective and flexible. Just let me know.

sabeechen commented 1 year ago

Just pushed a fix for another bug I created accidentally while refactoring, incorrect progress numbers.

I also wanted to mention that I don't have a lot of experience with C++ specifically compared to other languages, so a lot of this change is based on lessons from other languages or just how I've seen others do it. I should till be able to justify anything I'm proposing, though.

andreisperid commented 1 year ago

Amazing @sabeechen !

I'm reading here, mostly to learn (I never use OOP, but always wanted to) and also to keep up with the changes.

If you don't mind, I think that instead of "Carousel", we should use "DaisyWheel" as a naming convention.

I already renamed everything and was already planning to commit from here, what do you think?

sabeechen commented 1 year ago

I fixed another bug I found, printer would crash instead of reporting busy when it receives a command while printing. I also added some information in the /api/status response to report memory usage and uptime.

I've got my printer set up to print a random tag every minute and I'm logging its memory usage and uptime. I plant to let it run like that for maybe a few days to see if I accidentally introduced any kind of memory leak with this change. I want this firmware to be rock solid.

sabeechen commented 1 year ago

Yup I'm fine with that (Carousel -> DaisyWheel), I think you can just push directly to this PR but if not I'll figure out how to set that up.

andreisperid commented 1 year ago

I'm curious: how are you profiling the memory?

sabeechen commented 1 year ago

The ESP32 has some built in methods that I'm returning in the /api/status query that give coarse indicators for memory available and heap fragmentation. If you didn't know, heap fragmentation is when there is memory free but its broken into chunks so tiny they're basically unusable (I can explain further if desired).

I'm using Home Assistant to request a random tag every minute over the API and then every 20 seconds I have it configured to scrape the status endpoint for memory usage and uptime. Home Assistant has good functionality built-in to save a history of such values. I've only had it running for an hour or so but it looks good so far. image

andreisperid commented 1 year ago

Really cool. I think I got it, just like Tetris, we want everything fitting together 😄Thanks for the explanation!

I just made a few micro adjusts on the descriptions while figuring out the changes. See what you think, I just sided with the more objective ones you made.

Do you want to finish your test before merging?

sabeechen commented 1 year ago

Maybe wait a day? I just need it to go long enough to establish a clear trend. Its already simulated more tags than anyone could ever need so I would expect problems to show up quickly, or at least quickly compared to most memory leaks.

andreisperid commented 1 year ago

Alright, neat!

sabeechen commented 1 year ago

I've been running the test for many hours and not seen any indication of memory problems, so I think were good on that front.

I however now notice that my character and finish LED's aren't lighting up during printing. Mine have never worked right, I think I switched their polarity at some point and fried them or the cables got mangled during my many reassembly adventures.

Looking at the code now it looks like analogWrite is being used to set the brightness values. Its my understanding that this won't do PWM brightness control for them because LED's require a constant voltage to function, and they should actually be controlled using the ledc methods as this guide describes.

Maybe it shouldn't block this PR if its working the same way as before (I don't think I changed how they write values), but could you fact check me on this? IIUC the led's should never have functioned using analogWrite. If they're still working the same on your machine with this firmware it can be fixed later.

Knochi commented 1 year ago

In the world of arduino “analogWrite” does PWM when it targets a non analogOutput pin (DAC) so that should be fine.

In regards to memory, heap and stack there is a nice write up from Adafruit .

andreisperid commented 1 year ago

@sabeechen good news about the tests! And @Knochi thanks for the article, I'll take a look ;)

Now, strange about the LEDs. Mine here been working nicely.

If you switched the LEDs polarity and the supplied voltage is correct (ESP32 will only output 3.3V, so pretty safe) you will surely not have it damaged as it is a diode. Also, it should not conduct current in the opposite direction up to maybe 5V? Check a datasheet for a similar white LED reverse voltage (Vr).

You can visually test LEDs with a multimeter in the continuity test (diode) mode, it will supply the power needed.

LEDs work with an optimal fixed voltage (incl. assuring optical specs consistence). Then by varying the on/off cycle we have an optical illusion of brightness variance. You can see this trick visually when trying to use lower PWM values (say, less than 50 out of 255). If you move the LED around, you can see the fast blinking as the persistence of vision will not be so — persistent — meaning photons will not hit the same place in your retina and meaning separate and weaker impressions.

Check out the expected behavior for E-TKT LEDs:

Idle:

During — feed & char match:

During — press:

Finish:

andreisperid commented 1 year ago

I'm checking out the ledc methods. It seems that the analogWrite should not be working with ESP32. Until now, it has, I'm a bit confused now 🤔

I will test your fork here in my E-TKT to see if we are in the same page.

andreisperid commented 1 year ago

Yup, tested here and no LEDs!

This was the reason: image the line below was active

Tested here and it works perfectly with the analogWrite. Maybe this is because of the Arduino for ESP32, maybe it handles it under the hood..

However, some bugs have arisen:

I will take a look here, nice exercise to start working with the new structure

sabeechen commented 1 year ago

I did a little digging, this behavior didnt make sense to me.

In the world of arduino “analogWrite” does PWM when it targets a non analogOutput pin (DAC) so that should be fine.

I think that only applies to the "official" Arduino board definitions (ATmega, etc), it looks like the esp32 arduino board definition doesn't even include an analogWrite() method, at least there isn't one in the version this project currently uses (I didn't check others)

I'm checking out the ledc methods. It seems that the analogWrite should not be working with ESP32. Until now, it has, I'm a bit confused now 🤔

The analogWrite the E-TKT is using actually gets defined by the ESP32Servo library here. It looks like it sets up some default ledc parameters when used to try to mimic the way analogWrite works with the official Arduino library. That would explain why it still works. The ESP32's internal PMW timers have some finicky limitations, so id still like to do more digging here, but it seems to be working. At the very least we should be able to get better than 256 levels of PWM (a future change maybe).

I will take a look here, nice exercise to start working with the new structure

Thanks for that, based on what I'm seeing on my board I can't test the LED's properly yet (I'll sort it out eventually). If you're seeing correct bahevior on your board with your changes I'd call this PR done.

andreisperid commented 1 year ago

Alright and thanks for the explanation, now it makes sense!

And great job on the refactoring, it has been a great opportunity for me to learn!

If you want to discuss a more about and/or have some help on the LED, we can continue on an issue 😉