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 <[email protected]>
Signed-off-by: Mateusz Furga <[email protected]>
@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.

@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.

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

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

First of all: thanks for your contribution.

I kinda agree with previous reviews: still I don't think adding a single small NIF has such a big impact, but:

According to the documentation:

In AtomVM we don't have a similar distinction, so they would both return the same value. In order to avoid code duplication (and adding more C code: we love Erlang, not C ;) ) . I suggest implementing os:system_time() in Erlang as a wrapper to erlang:system_time().

There is a caveat here:

We have erlang:system_time/1 not erlang:system_time/0. That would require native units. I suggest to draft them as an alias to microsecond, and to provide a convert_time_unit (implemented in Erlang) that converts native (aka microseconds) to any time unit.

So we can have both erlang:system_time/0 and os:system_time/0 in pure Erlang.

The day we'll have such a distinction between os and erlang system_time, we'll likely do a C NIF, but right now it is not required.

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.

4 participants