search

omriharel / deej

Set app volumes with real sliders! deej is an Arduino & Go project to let you build your own hardware mixer for Windows and Linux
https://deej.rocks
MIT License
4.71k stars 433 forks source link

Add max_analog_read to config #80

Closed Neokil closed 5 months ago

Neokil commented 5 months ago

This new value allows you to define what the maximum value is that the analog read returns. By default it is set to 1023 but for example in case of a ESP32-WROOM it is 4095.

omriharel commented 5 months ago

Hey there @Neokil, thank you for your interest in contributing to deej.

As per the project's contribution guidelines, I'm currently not accepting PR submissions for deej's main fork. That being said, I encourage you to continue working on these changes to your heart's content in your own fork of the project.

Specifically in the context of this change, is there a reason that you'd prefer to do this on the client side as opposed to remap the values on the Arduino side?

Neokil commented 5 months ago

Hey, that's fine for me. Maybe it would be a good idea to put a link to the fork into the FAQ if someone hits the same issue.

Regarding why I did it there is just because you are already doing the mapping of the 0-1023 to percent there and it did not make sense to me to add another conversion upfront instead of making that one configurable. But you are right that would also be possible

omriharel commented 5 months ago

Understandable!

The thing the stood out to me when reviewing the code is that we're risking some unexpected or undefined behavior because there is no "sanity" checking of the config value (to ensure things like division by zero, etc. don't happen).

Of course adding these checks is possible, but I personally think it makes more sense as a device-side calculation (especially since there's a built-in map() function available for Arduino).

Regarding documentation of various forks and the things they add - I'll be honest, I'm still figuring out the best way to do this. I would want to avoid having a general list of "here's every fork that exists" live in the project documentation, but I might have a different approach for this at some point.

Neokil commented 5 months ago

I like that idea with the map-function. I think I will refactor my fork so the arduino-code will already provide a percentage-number, then there is no mapping required on client side