Conversation
- ripping out config/file support for now (needs complex cgo to solve) - adding on-screen panic logging support - adding on-screen logging support - adding CV and V/Oct value support - reducing required go version to minimum viable
- now using bootstrap system
- large list of bootstrap changes and fixes - new knobmenu - new quantizer - new screenbank - button / digital input handlers now support rising and falling edges - text alignment on display - hertz unit (frequency domain) - lerpround and inverselerp math functions - clock generator example - random skips example
Slimmer update with just bootstrap and 2 examples
- also allow users to access hardware revision singletons as they please
- add a little bit of testing
- document revision and hardware ids - clean up linting of hardware/rev1
- added testing
- add some testing - add revision detection
- fix issues with ui lifecycle
awonak
left a comment
awonak
left a comment
There was a problem hiding this comment.
Wow, I am thoroughly impressed with the contributions here! Thank you for breathing some life back into a passion project of mine that got sidelined. I hope this work will inspire others to contribute to the TinyGo firmware port as well, and I know that there are some features and ideas here we'd love to backport into the official firmware.
I am following some of the guidelines and conventions from the official EuroPi repo's contributing guide, so please take a look at this doc: https://github.com/Allen-Synthesis/EuroPi/blob/main/contributing.md#overview
I have started adding some comments, mostly observations from a quick glance and mostly conversational to better understand some of the design decisions. If there are any areas you'd like me to particularly focus on, please let me know.
I haven't yet flashed my EuroPi with this version of the firmware, but one of the major roadblocks I had previously hit was the poor accuracy of cv from the PWM output. I'm wondering if you had noticed the same issue and if your changes have helped improve the quality of PWM output?
bootstrap.go
Outdated
| return errors.New("no europi available") | ||
| } | ||
|
|
||
| Pi = e |
There was a problem hiding this comment.
[question] For my edification, what the purpose of having two separate variables for the EuroPi singleton? Why not just assign directly to Pi?
There was a problem hiding this comment.
The idea is that the bootstrap will setup and maintain the singleton while within specific segments of the bootstrap's lifecycle. It's available for applications to use (or to set themselves if they want to forego the bootstrap paradigm entirely) while within the healthy operating stages of the bootstrap lifecycle.
The bootstrap keeps around a copy of the EuroPi object (the e variable here) for things like the panic handler and shutdown processing. Nearing the end of the shutdown/panic processing, there's a moment where the Pi singleton might be unset, but the e value handed to the callbacks will still be available.
bootstrapoptions_features.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| DefaultMainLoopInterval time.Duration = time.Millisecond * 100 |
There was a problem hiding this comment.
[discussion] A default of 100 ms for main loop sleep time seems a lot to me. I have noticed if scripts put too much of a delay in the main loop, the knobs can feel sluggish. On the other hand, if Display.Show() is called too frequently, that will slow down the script (ideally Display.Show() should only be called if UI state has changed). I think this is a tricky balancing game that we should probably document well somewhere for script writers. These are decisions script authors will have to make based on the implementation details of their script.
There was a problem hiding this comment.
Indeed - I totally agree. 100 ms was a 'safe" default value decision made with very little consideration with regards to user experience on my point when I made it. In my day job and when working with UI in video games, this value is largely there to prevent code from running away and stealing all the frame budget, but not long enough to make players upset if it manages to make it out to production.
When it comes to EuroPi functionality within the Go bootstrap paradigm, it's intended that there will be at least 2 goroutines running simultaneously, the main loop (where CV/VOct is calculated and set) and the UI loop (where user inputs from knobs and buttons are handled and passed over to the main loop). This poses a bit of a problem, though, where a small understanding of multi-threading is required and care to avoid resource contention becomes more important. To combat this, I've made most of the EuroPi resources automatically avoid contention - though there is nothing that speaks directly about this to the consumers of the SDK.
I'll add some documentation on how to avoid resource contention between UI and main 'threads' and additionally come up with some documentation about how to tune the main and UI loops timers with points about tuning and caveats.
experimental/draw/colors.go
Outdated
| White = color.RGBA{255, 255, 255, 255} | ||
| Black = color.RGBA{255, 255, 255, 255} |
There was a problem hiding this comment.
[discussion] Should these be different? Is White the default draw pixel color and Black the negative/erase pixel color?
There was a problem hiding this comment.
They definitely need to be different - that's an 'oops' on my part and I'll get a fix in for it.
The intention for the Black color is to negate/erase pixel color. When a color screen becomes a reality on a future hardware design, we'll definitely need to consider how inversion occurs and what "erasing" of pixels means, especially when we have a 24bit+8bit alpha colorspace potentially available.
hardware/rev1/analoginput.go
Outdated
|
|
||
| // ReadCV returns the current read voltage as a CV value. | ||
| func (a *analoginput) ReadCV() units.CV { | ||
| // we can't use a.Percent() here, because we might get over 5.0 volts input |
There was a problem hiding this comment.
[discussion] I'm curious why we're limiting ReadCV to 5v when its capable of 10v?
There was a problem hiding this comment.
I mainly put it together to follow a common pattern in the industry where -5V .. +5V is the range of CV of all kinds and 0V .. 10V is the range of V/Octave.
If we followed the Doepfer A100 spec exactly, then this actually means:
- Audio/Noise = -5V .. +5V
- LFO CV = -2.5V .. +2.5V
- Envelope (ADSR) CV = 0V .. +8V
- Gate / Trigger / Clock CV = 0V or +5V
- V/Octave = 0V .. +10V
A generalized 0V .. +5V CV range felt like a happy median point for most use cases. I did add some other accessor functions to allow reading in different units or ranges of values.
I'm totally open to discussion about this and am happy to even fully implement the Doepfer A100 spec if we decide that's best.
Co-authored-by: Adam Wonak <adam.wonak@gmail.com>
europi proto doesn't have a screen, so can't display panic message. Co-authored-by: Adam Wonak <adam.wonak@gmail.com>
The main concern is that the RP2040 likes 32bit sizes for its floating point and most of the code is built around that (general use case performance is actually a major consideration point on this project) - with 32bit floats, we run into a problem of gaps in PWM values. It's not an impossible thing to solve, but it does mean reworking how duty cycle is calculated, focusing more on accuracy than fast calculation. Maybe we can replace the calc with a partial lookup table instead? That would certainly help where accuracy is concerned, however it would eat a bit of available memory/storage space (not that I've ever come across that concern in my testing/hacking) |
- rename things to be more clear - document undocumented calls - add TODO notes where necessary - fix an issue where the color Black wasn't. - mea culpa.
One of the issues with the organization layout is that if we are going to eventually run CI pipelines on the codebase for unit tests, then we'll need to put module projects (which have |
- add support for bipolar CV setting and getting - clean up knob bank a little
- fix numerous usability issues with offline debugger - fixed envelope mapping on analog and pwm devices
- simplify startup process - make detection more manual (require calling EnsureHardware()) - this prevents major init() leaks and race conditions - clean up screen/ui hardware detection - reduce required components to build to minimums - target europi prototype with build flag
- reduces chance of races
- see github.com/heucuva/europi-bootstrap - this slims down the firmware-specific code significantly
- simplify setup while allowing for more complex calibration
Cleanup, support for prototype, and additional improvements
|
Made a bunch of changes and improvements for memory utilization. Still need to make a pass on the documentation. |
2 participants
Preface
I'm really sorry this took so long to get ready for a real Pull Request. There were a lot of features I wanted to have available and things sort of spiraled out of hand a bit. I hope you understand and bear with me on this one, as I really do think this is the start of something great for the project. I promise that future PRs won't be behemoths like this one, but more pruned and targeted solutions.
Added
__debug_binto the.gitignoreso git doesn't try to add the VSCode debugging executable to the repo.GPIO6..GPIO9on Pico hardware.KnobBankfrom the Python EuroPi SDK.hal) system for interacting with hardware.Lerp,InverseLerp,Clamp, andQuantizer.unitsto support:CV- a normalized (0..1) representation of monopolar CV values ranging between 0 and 5 Volts.BipolarCV- a normalized (-1..1) representation of bipolar CV values ranging between -5 and 5 Volts. Note: EuroPi (revision 1) does not support Bipolar CV inputs/outputs, so this is largely unused.VOct- a representation of monopolar Volts per Octave values ranging between 0 and 10 Volts.Hertz- a representation of frequency values with a useful conversion to their peak-to-peak time intervals.revision0,europiproto, oreuropiprototype.context.Contextsupport to the various variants of the EuroPi hardware objects..Context()from the EuroPi hardware object to get the context interface. You can extend it with the functions found within thecontextpackage further, if you so choose.europi.Button(e europi.Hardware, idx int)- Get the button of indexidxfrom theeEuroPi hardware object, if it exists.europi.Knob(e europi.Hardware, idx int)- Get the knob of indexidxfrom theeEuroPi hardware object, if it exists.europi.Display(e europi.Hardware)- Get the primary display from theeEuroPi hardware object, if it exists.Remapperinterface to thelerppackage.TODOcomments throughout the code for adding EuroPi-X (revision2- aka 'rev2') future functionality.Fixed or Changed
init().Pivariable in the hardware revision subpackage becomes non-nil, et voila!Notes