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

Implement esp:get_default_mac/0 based on esp_efuse_mac_get_default #973

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

tovam
Copy link
Contributor

@tovam tovam commented Dec 7, 2023

Add get_default_mac function to retrieve the default MAC address from the EFUSE block of the ESP32 chip. This function utilizes the esp_efuse_mac_get_default API to access the factory-programmed MAC address.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

esp_err_t err = esp_efuse_mac_get_default(mac);
if (err != ESP_OK) {
ESP_LOGE(TAG, "Unable to read default mac. err=%i", err);
RAISE_ERROR(ERROR_ATOM);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit torn about this, because I can see that we made the same mistake in the definition of get_mac. What I would prefer is that we return {error, Reason :: term()}, where reason in this case is some sort of mnemonic atom, mapped from the esp_err_t.

The question is whether we make the fix here, or whether we follow up with a subsequent PR that addresses both functions.

Let's at least put a TODO comment in this code, so that we know to address it in the future, and file an issue in GitHub to that effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me. I'll add a TODO in this PR and create an issue about the API change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion here, I'd rather go with {ok, Result} | {error, Reason} already with this function.
So we reduce API changes in advance (and we make less boring future work for adapting existing functions also for us).

Copy link
Contributor Author

@tovam tovam Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, and I'll include the API change for get_mac in the current PR too, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest the following:

  • adopt the new API for esp:get_default_mac/0 for the reason I mentioned. It is a new function so no breaking API change will happen
  • keep get_mac/1 as is, since changing return type would break existing API, and we are approaching v0.6.0 so I'd like to keep existing API frozen.

Add `get_default_mac` function to retrieve the default MAC address from
the EFUSE block of the ESP32 chip. This function utilizes the
`esp_efuse_mac_get_default` API to access the factory-programmed MAC address.

Signed-off-by: tovam <tovam@users.noreply.github.com>
@tovam tovam force-pushed the add-esp32-get-default-mac branch from f37e789 to 3b779fe Compare December 10, 2023 23:27
@tovam
Copy link
Contributor Author

tovam commented Dec 10, 2023

I've just pushed an updated version, does the code look good to you?
I'm still getting familiar with the project's codebase but I believe it should be fine, I'll resolve the conflicts once you confirm that.

@bettio bettio merged commit 9a38346 into atomvm:master Dec 12, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants