xil-se / xildebug_sw

XilDebug is a CMSIS-DAP compliant debugger, UART bridge and power profiler all in one package.
5 stars 1 forks source link

Have a consistent getter/setter pattern #55

Open kbeckmann opened 6 years ago

kbeckmann commented 6 years ago

I messed up here a bit, sometimes i use something_set, other times it's set_something.

What should we use? something_set feels a bit more api-ish than set_something, right? @arturo182

arturo182 commented 6 years ago

I think it should be modulename_set_thing. I checked some of the drivers and it looks like only the power module is odd:

void power_dut_set_enabled(bool enabled);
err_t power_dut_get_enabled(bool *p_enabled);
err_t power_dut_ldo_set(uint32_t millivolt);
err_t power_dut_ldo_get(uint32_t *p_millivolt);

Should probably be:

void power_set_dut_enabled(bool enabled);
err_t power_get_dut_enabled(bool *p_enabled);
err_t power_set_dut_ldo(uint32_t millivolt);
err_t power_get_dut_ldo(uint32_t *p_millivolt);
kbeckmann commented 6 years ago

err_t power_set _dut_ldo Why space 😪

kbeckmann commented 6 years ago

After discussing with frazz and a few 🍻later, we think that power_dut_enabled_get makes the most sense. Reason is that if you sort the calls it all makes sense. Module > narrower scope > set/get. Get and set should be close to each other.

arturo182 commented 6 years ago

I don't agree, it signs like Yoda speak, "power duty enabled get, hihihi" https://youtu.be/5I671B4MCC0

kbeckmann commented 6 years ago

@EnJens we need your input 🤙

EnJens commented 6 years ago

I think it should end in set/get as it's the "functionality" of the function rather than what it works on

arturo182 commented 6 years ago

So we should have

adc_callback_set
max14662_value_set
max14662_bit_set
max14662_value_cached_get
max14662_value_get
mcp4018t_value_set
mcp4018t_value_get
uart_handle_get

?

That all sounds so wrong :/

kbeckmann commented 6 years ago

https://github.com/mossmann/hackrf/blob/master/firmware/common/max2837.h

They have some inconsistency there as well.. max2837_set_frequency vs max2837_reg_read/max2837_reg_write. Seems people mix these things up all the time.

arturo182 commented 6 years ago

I think this is the highest prio issue right now ;)