API interface contracts, odd pointer things, NULL's, and bugs.

[uC_Jon]

First let me say that I am very new to both C and microcontroller development, as in a few weeks/months worth of hands on and massive amounts of google searching (although I had a background in RPG programming, but that is very simplistic language in comparison).

My environment is from a clean install, and clean clone of esp-idf.

The devices I’m programming are ESP32C6’s.

My first two questions are specifically about the eeprom example as I’m using the new i2c_master code; which obviously is in flux as I’m using the “master” branch so I regularly delete and re-install as I test out various examples. My idea is after I have all the individual bits working by copy modifying the relevant examples I’ll then create a larger multifunction program using what I’ve learned from the individual examples.

So far I have a working LVGL display on one ESP32C6, another that reads from a MCP9808 temperature sensor, another that updates its time from NTP, and I’m currently working on one that will use a SD3031 external real time clock with battery backed memory.

For this current “project”, and the previous MCP9808 one, I used a mix of the i2c_eeprom example and the i2c_simple example. Both “work” as far as my code goes, but as I’m creating “device components” in a similar manor to the espressif online component registry I figured it would be worth the learning experience to make them as bullet proof as possible. In doing so I noticed a couple of possible bugs and a more “am I missing something” set of questions.

So what I think are “bugs” are as follows:

i2c_eeprom.c line 28: Shouldn’t the calloc be on the size of the i2c_eeprom_t not the size of the i2c_eeprom_handle_t ?

So it should read as follows:

Code: Select all

out_handle = (i2c_eeprom_handle_t)calloc(1, sizeof(i2c_eeprom_t));

i2c_eeprom.c example lines 48-53: Isn’t that a use after free bug? Shouldn’t the test and removal of the device be before the memory is freed so it should read as follows:

Code: Select all

err:
    if (out_handle->i2c_dev) {
        i2c_master_bus_rm_device(out_handle->i2c_dev);
    }
    if (out_handle) {
        free(out_handle);
    }
    return ret;

Or possibly the following:

Code: Select all

err:
    if (out_handle) {
        if (out_handle->i2c_dev) {
            i2c_master_bus_rm_device(out_handle->i2c_dev);
        }
        free(out_handle);
    }
    return ret;

If I am wrong on the above, please feel free to let me know why so I can understand why one way is chosen over the other; as I say I’m very new to this and I think the first one is defiantly a bug as when I created code similar to the uncorrected example (with a quite large struct) I ended up with unchecked exception errors but calloc’ing the struct, not the handle, fixed my problem (at least it no longer borked).



My next group of questions are a much more muddled and to do with the responsibility of API code and best practices so much more “feels” over “the language states this should be done”.

It took me a while to understand the idea behind using:

Code: Select all

typedef struct i2c_eeprom_t *i2c_eeprom_handle_t

And then also having to use the * on the *eeprom_handle at the end of the function definition

Code: Select all

esp_err_t i2c_eeprom_init(i2c_master_bus_handle_t bus_handle, const i2c_eeprom_config_t *eeprom_config, i2c_eeprom_handle_t *eeprom_handle);

If I’m correct in my understanding the function definition requires an explicit * and doesn’t “use”/”see” the implicit * in the typedef handle pointer to struct, or some other strange thing that I don’t understand?

It seems it requires a pointer to a typedef handle pointer to a struct. Its not seen as a pointer parameter to a struct so is not the same as using the alternative struct i2c_eeprom_t *eeprom_handle (I think, I may be wrong!) But if I am correct then the alternative would be something like the following:

Code: Select all

esp_err_t i2c_eeprom_init(i2c_master_bus_handle_t bus_handle, const i2c_eeprom_config_t *eeprom_config, struct i2c_eeprom_t *eeprom_handle);

Or possibly without the “struct” keyword if the definition i2c_eeprom_t is a normal (non-pointered) typedef?

Again, if my understanding is incorrect please let me know. Its quite hard to know the exact linguistic terms to get a valid answer from google; most of the pages it brings up are much more “c pointers for dummies” examples so don’t actually cover the specific case.

Anyway, once I had a rough understanding of what the code was doing (at least enough for my copy and modified code to run) I then came across another “oh, am I missing something?” question.

I guess there can a huge difference between reading code and getting an understanding (even if not totally correct; at least enough to get the gist of things) and what the API interface documentation and header declaration say should happen.

(Part of this is due to my taking a while to understand that there is a difference between a function declaration with a * and a * in a typedef and that, as far as I can tell, they are not the same “pointer”/"pass by reference" to allow updates parameter.)

In the i2c_master.c the various “add/new” functions all have a * to the *handle_t and as such they return a pointer to the data that is needed to be passed around to other functions. This I think I understand. And I think I can see how this allows for a completely unique and isolated set of data to a specific device on a specific bus and so on...

In some other functions in i2c_master the handles are passed as constants, such as the i2c_master_bus_add_device which gets passed a constant of the handle of the bus that it should add itself to and returns a handle pointer for the device that is added to the bus. I think I also understand this.

The first place that it seems that something is slightly amiss is that various functions within the i2c_master.c perform various tests on the passed parameters. The most obvious being testing for NULL, for example

Code: Select all

ESP_RETURN_ON_FALSE((handle != NULL) . . .

in i2c_master_bus_rm_device(i2c_master_dev_handle_t handle), yet the example doesn’t initialise the handle(s) to null and neither do the “new”/”add” functions on failure. To my mind either the examples should explicitly NULL the variable at its declaration or the functions that return a pointer on success should return NULL on error. Am I correct in this, or am I making an incorrect assumption and there is something I’m missing? Obviously the API contract is the contract, but it seems ill defined.

Now logically the code in say a “main” program should test the return code and not do something silly like call an add device to bus if the bus create wasn’t successful but then that makes the the NULL checks kind of redundant if the pointer isn’t set to a null at some point, either manually by the user/caller or automagically by the procedure that creates and returns a pointer to a struct on success. Going back to the original "bug" it should be modified (assuming I have the correct idea) to be:

Code: Select all

err:
    if (out_handle) {
        if (out_handle->i2c_dev) {
            i2c_master_bus_rm_device(out_handle->i2c_dev);
        }
        free(out_handle);
        out_handle=NULL;
    }
    *handle = out_handle; // out_handle will always be NULL here.
    return ret;

The other place something seems odd is within a function such as: i2c_master_bus_rm_device(i2c_master_dev_handle_t handle)
where it frees the handle and then NULL’s it, but as the function isn’t a pointer to a pointer to a struct (as in the add function) then NULL has no effect on the value as far as the caller is concerned. So either the caller has to make sure to remember to NULL on success, or the function should be declared:

Code: Select all

i2c_master_bus_rm_device(i2c_master_dev_handle_t *handle)

so that the NULLing of the pointer is passed back to the caller. Does that seem logical?

Both the declaration and the documentation point to the idea that the function should not be returning the new state of the pointer and its the responsibility of the caller to do the NULLing (or make sure the code never calls a function that needs a handle), so the NULLing in the rm_device function becomes redundant as well?

My gut feeling on the above is that the functions should do the tests as they currently do (if its null then its an error which is logical), but the add/new should also automagically NULL on error and pointer on success, and that the del/rm functions should be more consistent with what they return (over and above the esp_err_t) so should return the now NULLed pointers which would require a change to the interface (a pointer to the typedef pointer to the struct).

Again, let me say I’m very new to this… so there is a huge chance I’m missing something, or my install is wrong or something else entirely. Please fell free to correct anything I have wrong.

As far as the install, I deleted the ~/.espressif/ directory and also cleaned out then re-cloned the idf, re-install.sh'd the tools, and then re-found the esp-idf set up from within vscode, so I don’t think I’ve got some unset compile option.

And if I’m missing something, say that the caller should never trust the called except for the value of esp_err_t and the caller should handle all NULLing and logical control, then please let me know. As I say, I’m here to learn and so far I’m really enjoying the experimenting and learning.

[MicroController]

Wow, what a post
Just a quick partial answer:

If I’m correct in my understanding the function definition requires an explicit * and doesn’t “use”/”see” the implicit * in the typedef handle pointer to struct, or some other strange thing that I don’t understand?

It seems it requires a pointer to a typedef handle pointer to a struct. Its not seen as a pointer parameter to a struct so is not the same as using the alternative struct i2c_eeprom_t *eeprom_handle (I think, I may be wrong!) But if I am correct then the alternative would be something like the following:

You're almost correct there. But: The function accepts a pointer to a 'handle'; this is so that the function can return the new handle value at the pointer's target. The alternative/equivalent would in fact be

Code: Select all

struct i2c_eeprom_t **eeprom_handle

Notice the double *: eeprom_handle now is a pointer to a pointer to a struct, so the function can write a pointer to the struct (the 'handle' value) to the pointer's target.

[uC_Jon]

Wow, what a post

Yeah, a bit rambling but I've been quite stoked to see each (variously modified, from tiny changes to almost totally rewritten) example come to life. The IDF and examples are fantastic; on the whole they just work and the documentation is excellent. Each working change results in a little "squeeeee" of excitement from me

Its been a long time since I got a rush out of programming. It feels like I'm kid again, back in the 80's copying code from a magazine to program the 6502 BBC micro.

I guess It should really have been broken in to multiple posts. I'm sure I'll have many more questions, so next time a little less rambling.

You're almost correct there. But: The function accepts a pointer to a 'handle'; this is so that the function can return the new handle value at the pointer's target. The alternative/equivalent would in fact be

Code: Select all

struct i2c_eeprom_t **eeprom_handle

Notice the double *: eeprom_handle now is a pointer to a pointer to a struct, so the function can write a pointer to the struct (the 'handle' value) to the pointer's target.

I was so close .

I can see now why c is compared to whittling wood with a scalpel; its precision is great but you're likely to end up covered in blood!