Skip to content

Conversation

mfurga
Copy link

@mfurga mfurga commented Oct 15, 2025

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

Signed-off-by: Mateusz Furga <mateusz.furga@swmansion.com>
Signed-off-by: Mateusz Furga <mateusz.furga@swmansion.com>
@petermm
Copy link
Contributor

petermm commented Oct 15, 2025

Looks correct.

Fwiw atomvm is yet to really have the concept of native time unit, so I would probably avoid mentioning that.

Since we have erlang:system_time I do wonder if it's better for DRY/LEAN code to add nano_second support there and just call that function from os:system_time, and avoid the extra nif.

Signed-off-by: Mateusz Furga <mateusz.furga@swmansion.com>
@mfurga
Copy link
Author

mfurga commented Oct 16, 2025

IMHO, it's better to implement os:system_time as a nif, since it's already defined that way in OTP:
https://github.com/erlang/otp/blob/master/lib/kernel/src/os.erl#L220

@UncleGrumpy
Copy link
Collaborator

UncleGrumpy commented Oct 16, 2025

IMHO, it's better to implement os:system_time as a nif, since it's already defined that way in OTP: https://github.com/erlang/otp/blob/master/lib/kernel/src/os.erl#L220

I agree with @petermm, it is better to use a single nif implementation, and have os:system_time use the same nif as erlang:system_time, but @bettio will have to make the final decision here. The reason being that some platforms are already in need of size optimization (RP2 and STM32 specifically). We want to make sure that the results and error returns are consistent with OTP, but we cannot always afford to use the same implementation. Adding an another nif with duplicate functionality might come at the expense of sacrificing some peripheral support to devices later.

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