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

Tracking the renaming of APIs in C and Python target #1108

Closed
housengw opened this issue Apr 20, 2022 · 9 comments · Fixed by #1097
Closed

Tracking the renaming of APIs in C and Python target #1108

housengw opened this issue Apr 20, 2022 · 9 comments · Fixed by #1097
Assignees
Labels
c Related to C target python Related to the Python target

Comments

@housengw
Copy link
Contributor

housengw commented Apr 20, 2022

Related issues / PRs:
#1103
#1097
lf-lang/reactor-c#65
lf-lang/reactor-c#68
lf-lang/reactor-c#66

While working on the C target APIs, I figured it would help to create an issue to keep track of the renames.
All of the old names are marked as deprecated.
Format:

  1. old name -> new name means that the API is renamed from old name to new name. old name is still available in the current release, but will be removed in a future release.
  2. old name (deprecated) means that the old name has not been renamed, and will be removed in a future release.
  3. new name (new) means that the API is introduced in the linked PR.

C target:

  • SET related APIs:
    • SET -> lf_set
    • SET_ARRAY (deprecated)
    • SET_NEW (deprecated)
    • SET_NEW_ARRAY (deprecated)
    • SET_PRESENT (deprecated)
    • SET_TOKEN -> lf_set_token
    • SET_MODE -> lf_set_mode
    • lf_set_destructor (new)
    • lf_set_copy_constructor (new)
  • schedule related APIs:
    • schedule -> lf_schedule
    • schedule_int -> lf_schedule_int
    • schedule_token -> lf_schedule_token
    • schedule_copy -> lf_schedule_copy
    • schedule_value -> lf_schedule_value
    • check_deadline -> lf_check_deadline
  • tag_t related APIs (WIP):
    • compare_tags -> lf_tag_compare
    • get_elapsed_logical_time -> lf_time_logical_elapsed
    • get_logical_time -> lf_time_logical
    • get_current_tag -> lf_tag
    • get_microstep (deprecated)
    • get_physical_time -> lf_time_physical
    • get_elapsed_physical_time -> lf_time_physical_elapsed
    • set_physical_clock_offset -> lf_set_physical_clock_offset
    • get_start_time -> lf_time_start

Python target:
- get_current_tag -> lf.tag()
- compare_tags -> lf.tag_compare()
- get_microstep (deprecated)
- get_elapsed_logical_time -> lf.time.logical_elapsed()
- get_logical_time -> lf.time.logical()
- get_physical_time -> lf.time.physical()
- get_elapsed_physical_time -> lf.time.physical_elapsed()
- get_start_time -> lf.time.start()

@housengw housengw added the c Related to C target label Apr 20, 2022
@housengw housengw self-assigned this Apr 20, 2022
@Soroosh129
Copy link
Contributor

Soroosh129 commented Apr 20, 2022

convert_volatile_tag_to_nonvolatile and delay_tag are internal functions I think.

Regarding the tag_t related API, perhaps we have too many functions.
Instead of lf_elapsed_logical_time, lf_logical_time, lf_current_tag, lf_microstep, lf_physical_time, lf_elapsed_physical_time, would it be better to have something like tag_t lf_get_time(bool physical) that replaces them all?

Elapsed times can always be calculated by subtracting tag_t lf_start_time(), but also we could add a second argument to tag_t lf_get_time(bool physical, bool elapsed).

I can take on this task if we come to a consensus about it.

@Soroosh129
Copy link
Contributor

Soroosh129 commented Apr 21, 2022

After talking with @edwardalee, we came up with the following function to replace lf_elapsed_logical_time, lf_logical_time, lf_current_tag, lf_microstep, lf_physical_time, and lf_elapsed_physical_time:

/**
 * Return current time according to `flag`.
 * Possible values for flag:
 *     1 - LF_LOGICAL_TIME or LF_PHYSICAL_TIME
 *     2 - LF_ABSOLUTE_TIME or LF_ELAPSED_TIME
 * 1 and 2 can be combined using a bit-wise `|` operand (e.g., LF_LOGICAL_TIME | LF_ELAPSED_TIME).
 */
tag_t lf_get_time(int flag);

@lhstrh
Copy link
Member

lhstrh commented Apr 21, 2022

After talking with @edwardalee, we came up with the following function to replace lf_elapsed_logical_time, lf_logical_time, lf_current_tag, lf_microstep, lf_physical_time, and lf_elapsed_physical_time:

/**

 * Return current time according to `flag`.

 * Possible values for flag:

 *     1 - LF_LOGICAL_TIME or LF_PHYSICAL_TIME

 *     2 - LF_ABSOLUTE_TIME or LF_ELAPSED_TIME

 * 1 and 2 can be combined using a bit-wise `|` operand (e.g., LF_LOGICAL_TIME | LF_ELAPSED_TIME).

 */

tag_t lf_get_time(int flag);

This looks arcane to me. Why not use an enum + typedef. Int is not a good type for the argument, I think.

@Soroosh129
Copy link
Contributor

This looks arcane to me. Why not use an enum + typedef. Int is not a good type for the argument, I think.

Is there an elegant way to make enums work with one argument? With the flag version, you can have lf_get_time(LF_LOGICAL_TIME) for example.

@Soroosh129
Copy link
Contributor

I think I found a solution with enums:

/**
 * Represent different methods of reporting time 
 * (to be passed to `lf_get_time`).
 * 
 *  - LF_LOGICAL_TIME: Return current logical time.
 * 
 *  - LF_PHYSICAL_TIME: Return current physical time 
 *    (a platform-specific snapshot of the underlying 
 *    physical clock).
 * 
 *  - LF_ABSOLUTE_TIME: Return an absolute value
 *    for time (could be platform-specific, e.g., based 
 *    on epoch time).
 * 
 *  - LF_ELAPSED_TIME: Return time relative to the 
 *    start time of the program.
 */
enum _lf_time_flag {
    LF_LOGICAL_TIME = 0x1,
    LF_PHYSICAL_TIME = 0x2,
    LF_ABSOLUTE_TIME = 0x4,
    LF_ELAPSED_TIME = 0x8,
};
typedef enum _lf_time_flag lf_time_flag;

/**
 * Return current time according to `flag` 
 * (see `lf_time_flag`).
 * 
 * Flags can be combined using a bit-wise 
 * `|` operand (e.g., LF_LOGICAL_TIME | 
 * LF_ELAPSED_TIME) as long as the 
 * combination is valid.
 */

tag_t lf_get_time(lf_time_flag flag);

@housengw
Copy link
Contributor Author

housengw commented Apr 21, 2022

@edwardalee and I just discussed that it might be worth considering to merge the get_..._time functions with get_current_tag and get_microstep.

In that design, we would have the following lf_tag or lf_get_tag function:

enum _lf_time_flag {
    LF_LOGICAL,
    LF_PHYSICAL,
    LF_ELAPSED_LOGICAL,
    LF_ELAPSED_PHYSICAL,
    LF_START
};
typedef enum _lf_time_flag lf_time_flag;

tag_t lf_get_tag(lf_tag_flag flag);

Since enums in C are not scoped (enum fields with the same name collide even if the name of their enum structs are different, see stack overflow), we should prefix the enum fields with LF_

An example use case would be:

// getting physical time
lf_get_tag(LF_PHYSICAL).time;

// getting elapsed logical microstep
lf_get_tag(LF_ELAPSED_LOGICAL).microstep;

// getting the starting tag
lf_get_tag(LF_START)

I personally quite like this idea since it highlights the superdense time concept in LF

@edwardalee
Copy link
Collaborator

I'm leaning slightly for lf_tag over lf_get_tag, just because it's shorter. But I don't feel strongly about this.

@housengw
Copy link
Contributor Author

We currently do not handle physical time as tags (ex. functions like get_physical_time return an interval_t instead of a tag_t).

Will it be ok to assume that any physical time has a microstep of 0 as a tag? This way, lf_tag(LF_PHYSICAL) would return tag_t{.time = get_physical_time(), .microstep = 0}

@edwardalee
Copy link
Collaborator

edwardalee commented Apr 22, 2022 via email

@housengw housengw linked a pull request Apr 26, 2022 that will close this issue
@housengw housengw changed the title Tracking the renaming of APIs in C target Tracking the renaming of APIs in C and Python target Apr 27, 2022
@housengw housengw added the python Related to the Python target label Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c Related to C target python Related to the Python target
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants