Nuclei-Software / nuclei-sdk

Nuclei RISC-V Software Development Kit
https://doc.nucleisys.com/nuclei_sdk
Apache License 2.0
117 stars 50 forks source link

Can board name prefix be removed from common functions? #15

Closed tuupola closed 3 years ago

tuupola commented 3 years ago

Currently some of the common function names are prefixed with the board name.

void gd_eval_led_init(led_typedef_enum lednum);
void gd_eval_led_on(led_typedef_enum lednum);
void gd_eval_led_off(led_typedef_enum lednum);
void gd_eval_led_toggle(led_typedef_enum lednum);
void gd_eval_key_init(key_typedef_enum keynum, keymode_typedef_enum key_mode);
uint8_t gd_eval_key_state_get(key_typedef_enum key);
void gd_rvstar_led_init(led_typedef_enum lednum);
void gd_rvstar_led_on(led_typedef_enum lednum);
void gd_rvstar_led_off(led_typedef_enum lednum);
void gd_rvstar_led_toggle(led_typedef_enum lednum);
void gd_rvstar_key_init(key_typedef_enum keynum, keymode_typedef_enum keymode);
uint8_t gd_rvstar_key_state_get(key_typedef_enum keynum);

Looking at the source I could not find any technical reason for this. Function signatures and behaviour are the same. Could the function names be unified to be the following?

void gd_led_init(led_typedef_enum lednum);
void gd_led_on(led_typedef_enum lednum);
void gd_led_off(led_typedef_enum lednum);
void gd_led_toggle(led_typedef_enum lednum);
void gd_key_init(key_typedef_enum keynum, keymode_typedef_enum keymode);
uint8_t gd_key_state_get(key_typedef_enum keynum);

This way you could use the same code for different dev boards.

RomanBuchert commented 3 years ago
void gd_led_init(led_typedef_enum lednum);
void gd_led_on(led_typedef_enum lednum);
void gd_led_off(led_typedef_enum lednum);
void gd_led_toggle(led_typedef_enum lednum);
void gd_key_init(key_typedef_enum keynum, keymode_typedef_enum keymode);
uint8_t gd_key_state_get(key_typedef_enum keynum);

I would also prefer this implemetation, because the functions has to be implemented in every board version.

fanghuaqi commented 3 years ago

And for the led_typedef_enum, keymode_typedef_enum and keymode_typedef_enum enums, how about current enums.

/* exported types */
typedef enum
{
    LED1 = 0,
    LED2 = 1,
    LED3 = 2,
    LED4 = 3
} led_typedef_enum;

typedef enum
{
    KEY_A = 0,
    KEY_B = 1,
    KEY_C = 2,
    KEY_D = 3,
    KEY_CET = 4
} key_typedef_enum;

typedef enum
{
    KEY_MODE_GPIO = 0,
    KEY_MODE_EXTI = 1
} keymode_typedef_enum;
RomanBuchert commented 3 years ago

Wouldn't it be more uniform to use an underscore for the LED names too (LED_1)?

fanghuaqi commented 3 years ago

Wouldn't it be more uniform to use an underscore for the LED names too (LED_1)?

Yeah, It could be good, and to be compatiable with previous release, I will just add new LED_0, LED_1, LED_2, and add macros to define the gd_rvstar_led_init to be equal with gd_led_init, since we have some application code depends old style APIs.

#define gd_rvstar_led_init          gd_led_init
#define gd_rvstar_led_on            gd_led_on
#define gd_rvstar_led_off           gd_led_off
#define gd_rvstar_led_toggle        gd_led_toggle    
#define gd_rvstar_key_init          gd_key_init
#define gd_rvstar_key_state_get     gd_key_state_get   

By the way, any voluteer to implement this feature?

Thanks Huaqi

fanghuaqi commented 3 years ago

Fixed by #17