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

ESP32/NVS: return tagged tuples instead of just the result #741

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

bettio
Copy link
Collaborator

@bettio bettio commented Aug 7, 2023

Return either {ok, result} or error, instead of binary() or undefined.

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

libs/eavmlib/src/esp.erl Outdated Show resolved Hide resolved
Copy link
Collaborator

@fadushin fadushin left a comment

Choose a reason for hiding this comment

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

Just a few small corrections needed. I am okay with adding the new API.

Also, if we are adding a new API, then we should add something to the programmer’s guide.

Use term_binary_heap_size(size) macro in order to calculate required heap size
for binary.

Signed-off-by: Davide Bettio <[email protected]>
…esult

New fetch function returns either `{ok, result}` or `error`, instead of
`binary()` or `undefined`.

Signed-off-by: Davide Bettio <[email protected]>
libs/eavmlib/src/esp.erl Outdated Show resolved Hide resolved
Right now there are `fetch` and `get` function, `put` name matches better
current naming.

Signed-off-by: Davide Bettio <[email protected]>
Let's explicitly mention the namespace, rather having a default one, so
applications are encouraged using their own namespace, rather a kitchen
sink one.

Signed-off-by: Davide Bettio <[email protected]>
Copy link
Collaborator

@fadushin fadushin left a comment

Choose a reason for hiding this comment

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

We should correct network.erl, which is using one of the deprecated functions (see comment)

%%-----------------------------------------------------------------------------
%% @doc Equivalent to nvs_get_binary(?ATOMVM_NVS_NS, Key).
%% @deprecated Please do not use this function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that we use this function in network.erl line 348, so that module should probably be corrected to not use the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We only deprecated esp:nvs_get_binary/1, esp:nvs_get_binary/2 is still valid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah good point

@bettio bettio merged commit ce0be6c into atomvm:master Aug 9, 2023
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking platform:esp32 ESP32 platform specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants