impl new i2c driver interface of esp-idf 5.2 #397
Conversation
|
When I use the stacktrace |
|
It looks like a stack overflow to me... |
|
@ivmarkov any thoughts on the points in the init PR comment? I think the |
Limited access to internet right now. Will follow up when I have an opportunity |
|
@teamplayer3 I finally have some time now, so I can start looking into this. My first question would be, why not implementing the new driver in a completely separate module from the old driver? As all "new" drivers were implemented so far (ADC comes to mind)? The name of the new driver's module is not so important as long as it is separate from the old driver. Otherwise, we'll lose the old driver completely, and until we have the new driver working properly, we probably don't want this? |
|
Ahhhh, you did it except moving the existing driver to |
|
Some quick answers here (unfortunately I don't have answers for everything...):
We do both (a single driver and multiple drivers). I'm leaning towards not splitting, as long as it results in readable code and real code reuse. If the async driver uses a completely different code path from the sync driver or it needs some expensive extra state (say, a hidden FreeRtos task to "simulate" async behavior), then it is probably better to keep those separate.
As per above, I think we should keep the code base backwards compatible, i.e. the legacy driver for now should stay in place, under its existing name and module.
No good answer, but we have to support this for other drivers besides i2c. Thinking loud: what if we detect this at runtime, using a global atomic or a mutex? I.e. if the new driver is instantiated on at least one i2c peripheral, the old driver would fail to construct (with a runtime error or a panic)? And the other way around?
I think that's good enough.
No idea :)
No idea, need to look at the C API I guess...
Yes, a 0-sized type should do. |
src/i2c.rs
Outdated
|
|
||
| let _lock_guard = driver.acquire_bus().await; | ||
| enable_master_dev_isr_callback(handle, port)?; | ||
| esp!(unsafe { |
There was a problem hiding this comment.
Just to mention that this pattern - just like the same pattern in the SPI async code - is actually unsafe in the presence of core::mem::forget
Imagine that you have called write(...).await, the buf is passed to the C driver, but then you core::mem::forget the future returned by the write method. What would happen, is that the C driver would still use your reference to buf - yet - from the point of view of Rust, the reference is no longer in use (because you just forgot the future!) - hence - it will allow you to do whatever you want with the buf.
Unfortunately I don't have a great solution. Easiest is to mark the write method (and all its friends) with unsafe, as not core::mem::forget-ing the future is something that the user now has to obey, and it is not captured in the rust type system.
The problem is, the write method on the e-hal is however NOT marked with unsafe...
There was a problem hiding this comment.
If it is not clear, the real solution is owned buffers. In other words, the Rust driver itself should first copy the data into its own owned buffers, and pass references to the C drivers to its own buffers.
This comes with disadvantages though - as in - extra memory use, extra copying...
src/i2c.rs
Outdated
| esp!(unsafe { | ||
| i2c_master_transmit_receive( | ||
| handle, | ||
| bytes.as_ptr().cast(), |
src/i2c.rs
Outdated
| esp!(unsafe { | ||
| i2c_master_receive( | ||
| handle, | ||
| buffer.as_mut_ptr().cast(), |
src/i2c.rs
Outdated
| })?; | ||
|
|
||
| NOTIFIER[port as usize].wait().await; | ||
| disable_master_dev_isr_callback(handle)?; |
There was a problem hiding this comment.
This would never be called if i2c_master_receive returns an error. You probably don't want this?
src/i2c.rs
Outdated
| fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> { | ||
| I2cDriver::write(self, addr, bytes, BLOCK).map_err(to_i2c_err) | ||
| #[cfg(not(esp_idf_i2c_isr_iram_safe))] | ||
| pub struct OwnedAsyncI2cDeviceDriver<'d> { |
There was a problem hiding this comment.
Can you elaborate why we need the Owned version of the driver? Not clear to me...
Also, thinking loud, but to your earlier question of whether we need separate sync and async versions of the driver... my feeling is that it might be easier for that dirver particularly, if it is a single driver, with read_async / write_async, *_async extra methods...?
src/i2c.rs
Outdated
| })?; | ||
|
|
||
| NOTIFIER[port as usize].wait().await; | ||
| disable_master_dev_isr_callback(handle)?; |
There was a problem hiding this comment.
It is a bit unclear to me from the ESP IDF docs whether the transmit function can still return with ESP_ERR_TIMEOUT when you have the callback registered? Maybe we need to examine the source code?
Because if that's the case, then we need a more complex logic here, potentially involving a loop?
src/i2c.rs
Outdated
|
|
||
| #[cfg(not(esp32c2))] | ||
| unsafe impl<'d> Send for I2cSlaveDriver<'d> {} | ||
| todo!("How to block?"); |
There was a problem hiding this comment.
Roughly speaking (there are more details to this):
You need a blocking mutex and a condvar. You need to lock the blocking mutex and then wait on the condvar. Waiting on the condvar will temporarily unlock the mutex. In the callback handler, besides notifying the static async notification, you also need to lock the mutex and then notify the condvar.
As soon functions are linked in from both driver versions, an error is thrown at startup: This because of a check in This means we cannot use both drivers simultaneous. |
I see. Deleting the legacy driver is still not an option. At least not until (a) the new driver is proven reliable (b) the old ESP IDF 5 versions are no longer used. (Because the new driver was introduced after ESP IDF 5.0, right?) Is the new driver (and the old driver) somehow covered by an ESP IDF component? Or is there a CONF_ setting whether to use one or the other? If any of these is true, then we can explore this path. As much as I hate it, in the absence of one of the two options from above, we can introduce a feature for that then. Something like But let's first carefully check if we have a CONF_ or if one (or both) of the drivers are in fact in separate components? |
|
Actually... The above will happen ONLY if the code of the user is calling into BOTH drivers! |
|
Problem with |
If I remove the legacy mod from i2c, it works without a problem. If I only use the new driver in my main code, and don't have removed the legacy stuff in |
This does not sound right. This means something is calling into the legacy code. Or the linker is not pruning the legacy code, which would also be weird. |
That all sounds nice in theory but in practice we run over and over into the problem with all legacy drivers. The simple truth is that esp-idf forbid the usage of both at the same time while not providing a compile time solution to exclusively select a driver. The pain point is that in our compiled libespidf.elf we always have both symbols for new and old drivers, something that would not so easily happen in a pure C project since you only use the header you need. We discussed the point some time ago a bit, but my stance is that we still should try find a way ( for example with more exclusive imports of old and new drivers in the bindgen.h) to make a more clear cut at the root of the issue and not engineer around it by shifting the problem into manual labor - tripple check if anything aligns correctly with all our drivers so we don't accidental call into old or new api where we should not, |
Tests:
This check is done in this function: __attribute__((constructor))
static void check_i2c_driver_conflict(void)
{
// This function was declared as weak here. The new I2C driver has the implementation.
// So if the new I2C driver is not linked in, then `i2c_acquire_bus_handle()` should be NULL at runtime.
extern __attribute__((weak)) esp_err_t i2c_acquire_bus_handle(int port_num, void *i2c_new_bus, int mode);
if ((void *)i2c_acquire_bus_handle != NULL) {
ESP_EARLY_LOGE(I2C_TAG, "CONFLICT! driver_ng is not allowed to be used with this old driver");
abort();
}
ESP_EARLY_LOGW(I2C_TAG, "This driver is an old driver, please migrate your application code to adapt `driver/i2c_master.h`");
} |
|
I think one solution could look like the following: us providing "user defined" kconfigs that define appropriate compiletime flags. That flags can be consumed in the binding.h and would also automatically generate them as to rust cfg(). A normal user than could set them in their sdkconfig file selecting old or new, with one of them as the default. That would need some work on us providing the mechanism in the first place in the current embuild/esp-idf-sys dynamic but i think its not to far out. I can explore it if you think that would be a viable solution. |
This would be a nice thing solution which will align to the I think a quick solution to switch to the legacy version would be to add a |
|
@teamplayer3 - thanks for you analysis, but this is not complete yet? It is unpleasant - I can imagine - but I would go inspect the @Vollbrecht Not even sure where to start? Maybe these two comments:
It is not
Headers have nothing to do with what symbols you end up with in your ============== Please don't take the above as nit-picking. Really! The last thing I want is to demotivate folks by pointing at their (potential) mistakes. BUT: I'm absolutely NOT OK with introducing grandiose new solutions (like the user-defined "conf" thing) or even new features to problems that we still do not understand completely. |
@Vollbrecht @teamplayer3 I think I have a very good hypothesis for the above problem that needs a quick test: I'm betting and hoping that the problem will then disappear! Background: https://rustc-dev-guide.rust-lang.org/backend/libs-and-metadata.html Specifically the section which says: (... where codegen unit = one |
|
Unfortunately, it might not be as simple as I thought. To translate the above ^^^ in simple terms, it seems there is just no Now, these |
|
@teamplayer3 You are using xtensa or riscv MCU for your tests? |
@ivmarkov for tests I used riscv |
|
@teamplayer3 if you can do one more test whenever you have some time, sorry: can you compile in |
|
my example fails for me both with |
|
Whats the status on this? |
The root cause prohibiting the merge of the driver is solved, yet there is still feedback in the driver code itself that needs to be addressed. For one, I would appreciate if the old (4.4.X) driver is left untouched as much as possible (including in its current module as well as its location in the file), and the new 5.X driver is in its own sub-module. For one, that would make the review much easier as the current diff is very difficult to review. There were other issues as far as I remember, but the bottom line is somebody needs to volunteer and pick it up. The |
|
I can pick it up again. And see if I can make it more diff friendly. @kaaax0815 maybe you can test it, because I'm missing the test infrastructure currently. |
@teamplayer3 Just to mention that when saying that the root cause is solved, this unfortunately does not mean it is solved at the ESP-IDF level. We do need to introduce a feature toggle for switching between the legacy and the new driver similar to this one. But at least now we know why this is necessary in the first place (because of those pesky GCC C constructor functions that get pulled-in if we have any code compiled and referencing the legacy or the new driver - even if that code is pruned by the linker the GCC C constructors that were pulled by the pruned code would stay and be called). So if that means the legacy code needs to be surrounded with a |
|
I don't know if I'll be able to test this. As I am using Wokwi currently. And maybe the components I want to use use spi |
|
Currently, I have problems building the Compile Error: |
Upstream broke all nightly versions, because they changed c_char definition stuff. A patch to fix it for esp-idf should be already merged but we need to wait so it trickels down... |
|
i fixed it temporarily by using esp channel |
|
For now, I restricted the As I understand it, this applies to the callback function, which is called after completed transactions and must be set for a device which should support async operations. In other words, you could remove this restriction if the callback is reset for each transaction (read, write, writeRead) and the bus is blocked for asynchronous transactions by semaphore or mutex. But I'm not quite sure, it must be tested. This would lead to the next async transaction call gets blocked until the last is finished. I think this is completely fine as long the blocking mechanism is async. |
closes #388
Interface
Sync
Async
trait impls
I2cDriver: implsembedded_haltraits by creating a device driver internally by using default device config (transaction unimplemented!)AsyncI2cDriver: implsembedded_hal_asynctraits by creating a device driver internally by using default device config (transaction unimplemented!)I2cDeviceDriver: implsembedded_haltraits by ignoring the address args of the trait (transaction unimplemented!)AsyncI2cDeviceDriver: implsembedded_haltraits andembedded_hal_asynctraits by ignoring the address args of the trait (transaction unimplemented!)Open Questions:
trans_queue_depthconfig (this must be done at driver creation time) (as it is implemented by now, the implementation takes care of it, but if we combine the drivers it could be done by the user in the driver config and by adding a comment to indicate to increase it when doing async operations)trans_queue_depth> 1?I2cSlaveDriver?embedded_halneeds it within the type itself -> add a generic to the device driver whether it is 7 or 10 bit address)sync and async driver combined + address len
(Not really working like this. It could work if we call the driver impl something like
GenericI2cDriverand publish a type for the blocking driver astype I2cDriver<'d> = GenericI2cDriver<'d, AsyncDisabled>and for asynctype I2cDriver<'d> = GenericI2cDriver<'d, AsyncEnabled>pros
cons
GenericI2cDrivertypeother sync and async driver combination solution
Don't use type system. Depend on the runtime check and the thrown error by the esp-idf when using an async function if the bus is not configured correctly. The user must set the bus config correct to enable async capabilities.