Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malloc and immediate free of peerdata when adding ESPNOW peer #84

Open
GeorgesOatesLarsen opened this issue Jul 25, 2021 · 1 comment
Open

Comments

@GeorgesOatesLarsen
Copy link
Contributor

GeorgesOatesLarsen commented Jul 25, 2021

So both our current firmware, and the official ESP-IDF examples follow the following pattern:

    esp_now_peer_info_t *peer = malloc(sizeof(esp_now_peer_info_t));
    if (peer == NULL) {
        ESP_LOGE(TAG, "Malloc peer information fail");
        vSemaphoreDelete(s_example_espnow_queue);
        esp_now_deinit();
        return ESP_FAIL;
    }
    memset(peer, 0, sizeof(esp_now_peer_info_t));
    peer->channel = CONFIG_ESPNOW_CHANNEL;
    peer->ifidx = ESPNOW_WIFI_IF;
    peer->encrypt = false;
    memcpy(peer->peer_addr, s_example_broadcast_mac, ESP_NOW_ETH_ALEN);
    ESP_ERROR_CHECK( esp_now_add_peer(peer) );
    free(peer);

The very, very strange thing is that we are malloc this variable, then immediately freeing it after passing it into esp_now_add_peer.

This raises the question: Why bother with mallocing it at all...?

Is this, perhaps, to get around memory access restrictions across task boundary? That seems sketchy.

If it is safe to immediately free the variable, it should be safe to just leave it defined on the stack.

OFC, in practice, freed data does not necessarily get returned to the system. So even though the memory is freed, it might still operate as intended.

This just seems really sketchy.

Unfortunately, ESPNOW documentation makes no mention of whether memory needs to be retained by the caller. Good design principles would lead me to assume "no" but we cannot know for sure without documentation.

I am going to move forward under the assumption that keeping the data on the stack is fine. I'll report back here on if that creates problems.

@GeorgesOatesLarsen
Copy link
Contributor Author

I would also like to point out the ostensibly incorrect use of vSemaphoreDelete to delete a queue. This seems like a mistake in the example. : /

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant