Don't waste RAM in the low-level TWI Wire drivers #245
Conversation
Since twi_readFrom() and twi_writeTo() are both blocking functions, there is no need to allocate a special twi_masterBuffer. Doing so wastes valuable RAM, uses extra time to copy the data to the secondary buffer, and limits the transfer size to TWI_BUFFER_SIZE. Instead, it only needs a pointer to the buffer for the IRQ to use for the transfer. And, if asynchronous non-blocking functions are ever added, which will require a different API and callbacks, etc., then the existing txBuffer and rxBuffer for slave mode can just be used there too, since master mode and slave mode can't both be active at the same time.
|
Thank you so much for the PR! The Wire library is just a copy of the original Arduino Wire library. I'm aware of the duplicate buffers and always thought this was a dumb design decision from the original author(s), but I've never looked closely into how I can properly fix this. There are so many users that rely on this library and that it works properly. I'm busy today (kids, family, new years celebration; you know the deal), but I'll review and merge this PR in a few days. Meanwhile, you could submit the same PR at the official Arduino AVR repo, as this indeed saved lots of valuable memory. |
|
Sure, I will look into submitting it on the main AVR repo you linked. It might take a day or so, for me to get around to doing that, for similar reasons. I submitted it here, since I've been using PlatformIO (not the VSCode editor front-end, but just their Python back-end for setting up projects and the CMake build scripts) and the core they are using for the m328pb came from this MiniCore repo, so that's how I ended up here. The thing that led me to making this change was a little I2C Character Display CPU board I made for the common 128x64 OLED displays. That display puts a lot of overhead on the micro driving it, and so I offloaded that overhead to a m328pb, since it has dual I2C ports, to function as a character-based display instead of graphical, since many applications only need text or limited graphics. That way, the main processor only needs to send the text to display and not worry with font generation and pixels and such. When picking that micro, I didn't fully think through things and the fact that I would need a full frame buffer, which consumed half of the m328pb RAM before I even started. It's working, but I'm now down to doing tricks like combining my boolean variables into bits via STL bitsets and such to avoid heap/stack collisions. There are more memory saving opportunities with this library, but this one was a quick start that saved a ton and was simple to implement and had zero impact on functionality. The double-buffering between the outer Wire layer and the low-level TWI layer is still in place. I've tested it now on both processors involved on this project, one as a master-only and the other using dual ports with one as a slave and one as a master, so it's been tested. For sake of full disclosure, my testing was before the recent changes for the timeout logic was added. I rebased my work on top of that so the merge would be clean on your end and I don't see any conflicts between this change and the timeout logic changes, but I haven't yet tested the new timeout code that you merged a few days ago. |
Yes, graphical screens eats a lot of RAM. BTW which library are you using? I've used the u8g2 library along with a 1.3" SH1106 screen for a commercial product. This OLED was driven by an ATmega1284P, so RAM constraints were not really an issue.
This is very true. IIRC there are a total of four 32-byte buffers; two for TX and two for RX. This seems a little unnecessary, but it will require quite a bit of work to rewrite the code to fix this. It should be done sometime though because there are >=64 bytes of RAM to be saved, which is quite a lot for small microcontrollers like the ATmega48.
Thanks for testing it thoroughly! I'll try it on a few different chips. If everything appears to work just fine, which I expect it to do, I'll add your fix to my other cores as well.
The timeout logic shouldn't be affected by this at all, but I can always test with this, just to be sure. I've been quite hesitant to add the timeout functionality because most users/setups don't need timeouts because an i2c slave should never occupy the bus by forcing either line high or low for an extended amount of time. If this for some reason happens, the user should fix the issue rather than the symptom. It also occupies additional resources and memory, which may be an issue on smaller chips. But lots of users have requested timeout functionality the last year, so as a compromise I've added it as an optional feature that can be enabled if the user really needs it. |
I am using the u8g2 library as well. This is on a 2.42" SSD1306/SSD1309 display. Actually, RAM wasn't the original constraint for the graphical screen. This goes back a couple of years ago to the MIDI Interrupter I designed for my Musical Tesla Coils. That interrupter (so called because it "interrupts" the high voltage arc with musical note frequencies to play music) used a STM32F103 micro to do PPM (pin pulse method) music generation for up to 34 simultaneous notes. To do that, it required 34 very finely tuned output compare interrupts and a dozen timer overflow interrupts, not to mention the interrupts needed for USART and/or USB for receiving MIDI data. Or the code to read an SD Card when using MIDI data from a file. It was running at 72MHz and could barely keep up with the work load. I2C is a very slow interface and with the large amount of data needed to be sent to print simple text (all of the individual pixels for it) meant that it couldn't do any writing to the screen when playing more than a note or two -- causing the display to be very sporadic at best. That's when I decided to throw together a m328pb-based coprocessor to convert the screen from a graphical screen to a character/text-based screen. I used the m328pb because it had dual I2C ports (which would just drop right inline between the main micro and the display in the existing circuit) and I happened to have a bunch of them in my parts box from a previous interrupter I had designed that used four m328pb's running in parallel, each doing 6 simultaneous notes. The new STM32 version was a step up by having a single micro do 34 notes vs 4 micros to do 24 notes. That solved the problem and, even though the code still had to be careful about when it did screen updates, it was able to write enough meaningful data to the screen while playing its full note suite. I first ran into the memory issues on the m328pb during that project and after converting my booleans to STL bitsets and being careful with the coding, I managed to get it to fit and not have a heap/stack collision with a safety margin of about a half-dozen bytes. And this worked great until about a month ago. About a month ago, I designed a simple tester for the bridge driver circuit for the coil using a m328pb. And since I already had the I2C character display, I figured I'd just plug it on and use it, with the secondary m328pb on it in the same coprocessor configuration. It was supposed to have been one of those quick one-day or one-weekend projects (I wasn't counting on Murphy's Law to intervene). The problems started when I had to recompile the code for the display processor. The display board I used on the STM32 was running the m328pb at 3.3V and 12MHz, since both the STM32 is 3.3V and the display I was using only supported 3.3V. But, on this new tester board, I was planning to run the display at 5V and wanted to use its full 20MHz, since it was talking to a m328pb on the tester that was also running at 5V. Plus, this was a slightly different display that supported 5V as well as 3.3V. But, this meant needing to recompile the display code to work at the higher CPU clock. Well, there was a new AVR toolchain on PlatformIO that took the compiler from gcc 5.4.0 to 7.3.0. I don't know what all they changed there, but IMO, 7.3.0 is quite inefficient compared to 5.4.0. The first problem I ran into was that the 13V voltage inverter on the display that drives the OLEDs was generating a ton of noise, so much so that it was causing oscillator startup issues on the m328pb when trying to run at 20MHz. The PB is very very sensitive to oscillator startup problems, compared to the P and many other micros out there. So, I ended up bricking two m328pb chips (as oscillator startup problems causes ISP programming to write garbage to the micro and depending on what values get written to the fuses, you may not be able to talk to the micro anymore). So, I had to drop back to 16MHz on the display board. But then, it still wouldn't function. Something in my testing led me to believe it was RAM related. That's when I found this extra buffer in the TWI code. This code change actually made it work fine with the SSD1309 driver with the gcc 7.3.0 compiler, but when I built the SSD1306 driver with that compiler, it was getting random garbage on the screen from even more RAM collision issues with the stack. I ended up switching from the 7.3.0 compiler back to 5.4.0 and everything worked perfectly. So for anyone reading this, if you are tight on RAM and are using the 7.3.0 compiler and are experiencing weird problems that seem to be memory collision/corruption related, try dropping back to an older compiler. Also, my PCB for the display coprocessor CPU is on OSHPark for anyone interested. Anyway, I finally got it all running, with the display coprocessor at 5V and 16MHz, using the 5.4.0 compiler. And the tester board running 5V at 20MHz (since I could unplug the display when programming it and not brick it), using the 7.3.0 compiler (since the tester itself still has plenty of RAM). BTW, I actually have a separate repo with just this Wire/Wire1 code in it with even more modifications to save more RAM. Much of it isn't as elegant. For example, I replaced all of the function pointers for the two TWI interfaces (which requires 2-bytes of RAM per function per interface) with switch statements and a single byte of RAM to hold the interface index. It doesn't read as elegantly and I think uses a few more bytes of flash than this implementation, but does free up some very valuable RAM. But, the code in this PR to get rid of the totally unnecessary 5th buffer in each TWI port was independent and easy to isolate, which is why I submitted it here in this upstream PR. I agree with your position on the timeout logic. There is, however, one possible use-case that I've seen where it might be useful and something other than a slave device extending the bus timing, and that's wiring issues and hardware problems. I've seen cases where a bad cable causes a pull-up resistor to get disconnected and cause an I2C master to hang. Having a timeout would allow the device to recover. This could be of safety concern, depending on the application. |
|
I interesting to read about how Tesla coils can play music! I've seen a big one play all kinds of songs in real life, but I've never thought there's that much more to it than regular PWM.
I know PlatformIO is highly configurable, but how do you tell PlatformIO to use avr-gcc 5.4.0 instead of 7.3.0? I'm also using PlatformIO quite a lot, but only for large, commercial projects where there are lots of external libraries that need to stay in sync.
The absolute dumbest thing Atmel/Microchip did was to remove the "Full swing oscillator option" that previous AVRs had. With this option, one sacrificed a little efficiency and gained instead a very rugged crystal driver that could pretty much drive anything. I designed a capacitive touch panel for commercial use, based on the very similar ATmega324PB. It ran at 8 MH using an external (Murata branded) resonator with built-in capacitors, as this lowered the number of components on the board, and thus BOM and assembly cost. The BIG mistake I did was that I hadn't realized how weak the 324PB/328PB were at driving external oscillators. Without thinking much about it, I chose the resonator I've used with the 1284P for years, which had internal 22p or 27p capacitors. This is too much for the 324PB to handle, and what happens was that the clock would eventually stop oscillating and the device just freeze. However, this never happened during the R&D phase, and only happen to devices after several weeks with 24/7 operation, and of course, shipped to other European countries (I'm based in Norway). We ended up having to recall all of them and manually replace all resonators with a more appropriate one with 10p internal capacitance instead. I also redesigned the board to use a crystal and external caps, just in case the assembly house had some old stock they figured would fit this board.
I'm aware that the Wire1 implementation isn't the nicest. It's written by a contributor, and it's done this way to make it compatible with existing libraries that expect an object from the
Good point! I should update the PlatformIO documentation and specify how to enable TWI timeouts. |
And actually, I realized I skipped one stage of history. The four processors in parallel was the 328P, as they could do only 6 notes each (3 timers, each with two output compares). Then there was a dual 328PB version, each with 12 notes, since it doubles the number of timers. And then the 34 note STM32F103 version, which I sometimes run two, one per coil, with two coils, and play it in stereo. I'm currently experimenting with combining a STM32F446 micro with a VS1053B audio codec/DSP, and a Spartan 6 FPGA, to try and achieve a high quality direct audio drive, plus experiment with PIM (pulse interleave method) synthesis in addition to the current PPM (pin pulse method). My interrupters are a bit unique. Most Tesla Coil setups can only do 3 or 4 simultaneous notes max. But, being a musician as well, one of my goals is to get the best quality sound in addition to longest arcs. They are a bit in opposition, because more simultaneous notes causes the arc to branch off with each individual frequency, rather than forming one single long arc. It's a bit more than just PWM because with a DRSSTC (dual-resonant solid-state tesla coil), the maximum pulse width is in the microseconds and is only 2-3% duty cycle -- so very short in PWM terms -- and especially once you start combining multiple simultaneous notes. If interested, you can see some of my coils running here: https://mediadrop.dewtronics.com/media/categories/tesla-coil
Locking in a specific toolchain is similar to locking a specific library dependency. In much the way that you add the "@" version information at the end of the library name, you add a The name and version number should match the detail in the And if you have completely custom packages, whether its frameworks, toolchains, or a full platform, you can even set up your own local webserver to host them as tarballs and create a
EXACTLY!! I'm glad to know I'm not the only one struggling with the clock oscillator on the PB. The very first custom circuit I did that used a PB had clock start issues and somehow, after programming it once or twice, managed to use up all of the flash write cycles in a number of locations scattered throughout the flash. It didn't brick it, like these recent ones with the display board, as you could still connect with it and program it, only it couldn't retain half the data. I contacted them to report it as a flaw in the micro, because the internal flash write logic should at worst case switch back to the internal oscillators for their write timing. Their "answer" was to just send me like ten free samples of the micro to make up for my trouble. The funny part (or perhaps sad part) was they also included some stickers in the shipment with the micros (company logo promotional type stickers). And the customs declaration form had the stickers valued at more than twice the value of all the micros they sent combined. If that declared value was their actual cost for the micros (and it probably was), then their markup on those micros is absolutely enormous.
Yep, I ran into that same issue before too. And trying to do a virtual base class doesn't work either because having all of the virtual functions chews up enormous amounts of RAM for each object instance to hold the vtable. One of my goals with the Wire libraries is to see if it's possible to define a structure for all of the I/O registers and just set a single pointer to the structure in the class. This will work only if all of the registers are in the same sequence and have the same values, only with different starting offsets. And I think that is actually true on the PB. This is how the STM32 world does theirs -- each peripheral has a base address for its registers and then the driver is identical for each of the peripherals. In the meantime, I'm going to try this afternoon to do that other PR for the official AVR upstream repo for this change. |
Thanks for the additional details! I watched a video, and your tesla coil does indeed sound really cool, even though one would have to experience it live to get the true sound. I'm not sure if it's relevant for what you're trying to do, but for audio processing my goto chip is the ADAU1701. When sourced from the far east it's ridiculously cheap, it's very easy to program (using SigmaStudio), and best of all, you can have complete control and change pretty much all parameters real time using my SigmaDSP library. When not playing around with Arduino source code, I'm a hardware engineer by profession, and I mostly design electronics that goes into audio amplifiers, transmitters, receivers and cross points. I've used the ADAU1701 in a ton of different product because of its simplicity and ease of use. For cheap, configurable and relatively powerful, distributed DSP audio processing, the ADAU1701 is really, really hard to beat. It has two built-in audio ADCs, and four DACs, but one can also add 4 i2s inputs (2 channels * 4) and 4 i2s outputs.
Thanks for the info! I'll play around with it to see how much better the 5.4.0 compiler really is.
Thank you! I really appreciate your time an effort. BTW I dug a bit deeper into the Wire/twi source code, and it appears that |
Thanks for the heads-up on this chip. Years ago, I worked a bit with several Analog Device DSPs, but nothing recently and I hadn't heard of this specific chip. I will definitely look into it!
I think it will depend on the specific project. I've used the newer 7.3.0 compiler for some things (still on the m328pb) without any issues and it does have newer C++ standard support. In fact, on one project, I used that aspect of it to be able to calculate a CRC at compile-time (not run-time) for an EEPROM default settings image and output an .eep file that was ready to program into a m328pb without needing external post-processing scripts, by using constexpr function support that's now part of the C++ standard. It's just that something seems to break down with the newer compiler on processors with less resources when those resources start getting tight. I've done some disassembly of the code and simple comparisons, but not enough to know the exact issue. Speaking of which, if you ever need a code-seeking disassembler with a fuzzy-function-analyzer for the AVRs, check out my generic code-seeking disassembler.
I don't know that the txBuffer and rxBuffer will be so easy to eliminate or combine. Unlike the masterBuffer that this PR removes, which was completely unnecessary since the master send/receive functions are blocking, the slave send/receive logic is asynchronous and certainly benefits from the double-buffering, allowing the application to process one message while the next message is being received. Instead of just combining the two buffers, I think the better solution, long term, would be to change the buffers on the |
|
I copied the contents of this PR over to a PlatformIO project where I'm using an ATmega1284P. It's a custom-designed 2x200W (TPA3255 based) amp where the microcontroller controls a DSP (ADAU1701), an external i2s audio delay IC (TPA5050), an external EEPROM IC (24LC256) a capacitive touch IC (AT42QT1070) and an SH1106 based OLED screen. All this is controlled over i2c, and this PR works perfectly fine with all these components. Thank you so much for the PR and for the interesting discussion. Feel free to open similar PRs for MightyCore, and MegaCore as well! And I'm sure other repos like the official Arduino one and ATtinyCore will benefit from this as well. |
|
Since it was trivial to use Thanks for testing and merging these and making the cores better. I too have enjoyed our side-discussions. And regarding our side discussions, I have one more question for you... Do you have any experience with doing an audio compressor via DSP? Not data compression, but audio compression with attack, hold, and release properties, etc., that can detect signal level and reduce it? Sort of like an AGC or automatic level control. If so, can you point me in the correct direction to research it? I've done similar functions with pure-analog circuits using opamps and such, but this needs to be in a DSP/FPGA to fit in with the rest of the system. I think that's the next piece I need for my direct audio drive circuit for my Tesla Coils to better match the 16-bit PCM audio levels to the 1-bit PDM drive level needed for the coil. |
Yes, I've used a DSP compressor "block" (a graphical component that you can control over i2c) when I designed the hardware and firmware for a control panel with a microphone input. I'm currently at home where I only have a mac, so I'm not able to open the SigmaStudio project, as it's a Windows-only software. By I can provide some more information and some screenshots tomorrow. I added the compressor in case the user screams in the mic or drops. It's pretty much impossible to distort the signal as long as the compressor matches the microphone (active with phantom power, dynamic etc.). But it's perfectly fine for audio use as well. Check out the SigmaDSP 8_Compressor_RMS example if you want to see how the Arduino control code is implemented and what the DSP program looks like. Serial.println(F("Threshold = +6dB, Ratio = 50, RMS TC = 72ms, Hold = 0ms, Decay = 869ms, Post gain = 0dB"));
comp.threshold = 6;
comp.ratio = 50;
comp.rms_tc = 72;
comp.hold = 0;
comp.decay = 869;
comp.postgain = 0;
dsp.compressorRMS(MOD_COMPRESSOR_ALG0_TWOCHANNELSINGLEDETECTALGFIX20_ADDR, comp); |
|
Thanks for the input. I managed to get SigmaStudio installed and running here on Wine in Linux. However, the project you linked seems to have a 0 byte |
|
You know ... we should probably move this discussion somewhere more appropriate anyway, I will go open an issue on that SigmaDSP repo regarding the zero-byte project file and we can at least continue this in a repo related to DSPs... |
Since
twi_readFrom()andtwi_writeTo()are both blocking functions, there is no need to allocate a special twi_masterBuffer. Doing so wastes valuable RAM, uses extra time to copy the data to the secondary buffer, and limits the transfer size to TWI_BUFFER_SIZE. Instead, it only needs a pointer to the buffer for the IRQ to use for the transfer.And, if asynchronous non-blocking functions are ever added, which will require a different API and callbacks, etc., then the existing txBuffer and rxBuffer for slave mode can just be used there too, since master mode and slave mode can't both be active at the same time.
When using both Wire and Wire1, this frees nearly 64 bytes of RAM, which is huge on a micro that only has 2K of RAM and you are down to counting every available bit for your application.