Open mcells opened 1 month ago
Hi @mcells, thanks a lot for this contribution and all the effort!
I see its still just a draft, but I don't know if we'll be able to merge it that soon...
at the moment it is making many test cases fail, not just ESP32 related ones. We'd definately have to get the code separated out in a way that it affects only ESP32 and no other architectures, and within ESP32 only the supported chips.
while we have a structure for this (for example in stm32) I have to say that we're discussing how to improve this in the future - it turns out current sensing is both more complicated, and has more optionality than PWM, so I think we would better capture the needs with an abstraction more like the sensors, based on classes, where users can "plug in" different types of current sensing at compile time and the whole process is more under user control.
so I would actually see this more in that future, as a "ESP32CurrentSenseI2SDriver" available to those that want/need it
and because it supports only a subset of ESP32 and has specific advantages for certain use cases like HFI, and also because you say "The code could just stop working any time." I would personally see it as part of the SimpleFOC Drivers repository, where we put code that is hardware specific and/or less supported than the main library.
another point to make is that ESP32 IDF 3.0 for Arduino is around the corner, with alpha releases already available and required for ESP32 C6 for example, and this new version of the framework brings many incompatible API changes... so we're about to have to upgrade all the ESP32 code in a major way, and its not the optimal time to extend the codebase in complex ways. So also from this point of view I would like to wait a bit before we attempt to merge anything anywhere...
I don't want to sound discouraging, the effort and work is really appreciated! We have a subset of users very interested in HFI, as you may know, and so this will indeed be interesting to people...
What do you say? Is it ok if we take it slowly on this, wait for ESP32 IDF 3.0 to come out in final release, and then work towards putting into SimpleFOC 3.0 with a better current sense driver architecture and possibly a API for HFI stuff?
Seems the test failures are mostly related to adding the conversion function call in the inline class, while the other platforms don't have this function defined. https://github.com/mcells/Arduino-FOC/commit/b68fedfe709c683aa3c3e464e01d1bd879f38d39
PhaseCurrent_s InlineCurrentSense::getPhaseCurrents(){
PhaseCurrent_s current;
_startADC3PinConversionInline(); // <<<<<<
current.a = (!_isset(pinA)) ? 0 : (_readADCVoltageInline(pinA, params) - offset_ia)*gain_a;// amps
current.b = (!_isset(pinB)) ? 0 : (_readADCVoltageInline(pinB, params) - offset_ib)*gain_b;// amps
current.c = (!_isset(pinC)) ? 0 : (_readADCVoltageInline(pinC, params) - offset_ic)*gain_c; // amps
Introduction
First, I am not certain if this is the right repo to put the code when it is ready for release, as it is quite HW-specific and has some constraints, and we already have a working current sense solution in place. Secondly, this is just referencing my developement branch, so the code has debugging options, some outdated comments and code, and is generally not very pretty.
Nonetheless, I want to get this out there for two reasons: On the one hand to possibly receive some opinions on the feasability of integrating this in the library at some point, on the other hand, I feel this could be useful for some people, be it users of the library, or people working with the ESP32´s adc, looking for information.
On the driver
This driver uses the (original) ESP32´s continous adc sampling mode, where the I2S peripheral connects to the adc and clocks out samples at a configurable frequency without using cpu time. This means that the adc sampling happens asynchronusly and non-blocking. Samples get pushed into the I2S´s 32-large FIFO automatically, where we only need to do a bit of bitshifting and save them.
This leads to the following usecases I implemented here:
The driver works (for me at least) and does definitely improve performance. (Loop speed and signal quality)
There are however some caveats to all this:
analogRead()
on any of the ADC1 channels, as this will most probably break the setup, cause crashes, or just not work. I´ve not tried it though.readfifo()
method. It´s not so much voodoo, but more guesswork as to how stuff behaves in edge cases. (No help looking in the TRM, as many Registers are just mentioned and not explained)