Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break out firmware into multiple classes. #46

Merged
merged 8 commits into from May 4, 2023

Conversation

sabeechen
Copy link
Contributor

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

  • Classes are based roughly on functionality, it has a pretty clear dependency chain.
  • Consolidated most of the #defines into a new file, Configuration.h, and used values for each #define to avoid having uncompiled code (a common source of errors).
  • Everything is clang-formatted based on Google style. To also format using clang, you'll need to install it from here.
  • Each class is relatively small and more-or-less a copy-paste from he original code, except for E-TKT and Network, which were both significantly reworked. I think this will make it easier to adapt to different hardware in the future, if desired.
  • Network.h handles both the server and WiFi stuff. Each command (cut, reel, tag, etc) has been broken out into its own api handler with its own validation.
  • ETKT.h is where all the logic for handling commands now lives. I also change the concurrency model to have the webserver enqueue a command to be run and then the device processes it by waiting for a signal in loop() (no more tasks deleting themselves). I think this will work better if we move forward with handling a print queue, it would be almost trivial to implement now. As a side effect, I believe I've also resolved any potential concurrency bugs that could be caused by using the API directly.
  • Carousel.h and Character.h have been added and written in a way that I think will make it easy to add different character sets if desired.

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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

@sabeechen
Copy link
Contributor Author

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
Copy link
Owner

andreisperid commented May 3, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

Alright, neat!

@andreisperid andreisperid modified the milestones: v1.3.0, v1.2.0 May 4, 2023
@sabeechen
Copy link
Contributor Author

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
Copy link
Contributor

Knochi commented May 4, 2023

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
Copy link
Owner

andreisperid commented May 4, 2023

@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:

  • character LED: off
  • finish LED: off

During — feed & char match:

  • character LED: partial brightness (just to see the letters on the daisy wheel)
  • finish LED: off

During — press:

  • character LED: full brightness
  • finish LED: off

Finish:

  • character LED: off
  • finish LED: blink, then keep fully on, then fade away gradually

@andreisperid
Copy link
Owner

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
Copy link
Owner

andreisperid commented May 4, 2023

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:

  • after printing a label, the ledChar stays half on
  • on feed, the ledFinish stays on half on

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

@sabeechen
Copy link
Contributor Author

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
Copy link
Owner

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 😉

@andreisperid andreisperid merged commit 8846313 into andreisperid:main May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants