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

Simplify leaf adding with one-line commands #36

Open
adamoferro opened this issue Jan 16, 2022 · 5 comments
Open

Simplify leaf adding with one-line commands #36

adamoferro opened this issue Jan 16, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@adamoferro
Copy link
Collaborator

When you want to add a leaf you need to use several commands which could probably be summarized in simple one-line ones.

Example.
To add a moisture capacity sensor this is the way now:

//creates the moisture sensor
	moist = gn_leaf_create(node, "moist", gn_capacitive_moisture_sensor_config,
			4096);
	//set the channel 4
	gn_leaf_param_init_double(moist, GN_CMS_PARAM_ADC_CHANNEL, 4); //GPIO12
	//set update time
	gn_leaf_param_init_double(moist, GN_CMS_PARAM_UPDATE_TIME_SEC, 5);
	//set initial status to active (on)
	gn_leaf_param_init_bool(moist, GN_CMS_PARAM_ACTIVE, true);

It could be summarized with something like:

	moist = gn_cms_fastcreate(node, "moist", 4, 5)

where implicitly "under the hood" all the other parameters are set. E.g., probably nobody would change that "4096" and a standard user when creates a leaf wants it active.

It could be:

  1. a function defined in each leaf (like in the example above), or
  2. if possible, a general function taking in input also the leaf type... but different leaves have different parameters, so it may be not straightforward

In case 1, leaf should have a standard and common "name" to use in every function/parameter definition/file names, in order to make it easy to know the name of the "fastcreate" function.
For example capacity moisture sensors have file names and config called "capacity_moisture_sensor" but parameters called "CMS". This may generate confusion.
Temperature ds18b20 sensors, instead, maintain the same name everywhere.

@adamoferro adamoferro added the enhancement New feature or request label Jan 16, 2022
@ogghst
Copy link
Owner

ogghst commented Jan 17, 2022

Thanks for the suggestion. As always, there should be a tradeoff between simplicity and flexibility.

What about a combination of struct and generic coding? like:

gn_leaf_definition_t leaf_definition = {
.node = node,
.name = "temperature"
.stacksize = 4096
...
}

temp = gn_leaf_create_from_definition (leaf_definition, GN_DOUBLE_TYPE, GN_CMS_PARAM_ADC_CHANNEL, 4, GN_DOUBLE_TYPE, GN_CMS_PARAM_UPDATE_TIME_SEC, 5, GN_BOOL_TYPE, GN_CMS_PARAM_ACTIVE, true);

This will achieve those goals:

  • leaf definition can skip some parameters, like stacksize could be a default if not specified
  • parameters initialization could be in a single row by adding type-key-value tuples.
  • no need to build customized creation functions in leaves code

The drawback is that an user can potentially create bugs or crashes if the parameter sequence is not respected

@adamoferro
Copy link
Collaborator Author

The solution you proposed is very flexible but in my opinion is even more difficult to understand for a beginner user than the current method :)

Let's see it from another perspective.

Now when you create a leaf you use "standard" functions (_init_create, _init_double, etc.) in a "specific" combination that depends on the leaf type. So the way you create a leaf cannot be considered "generic" because it's leaf-dependent.
A user needs:

  • to know which standard functions to use (_init_create, _init_double, etc.), the coded names of the parameters to set (GN_CMS_PARAM_ADC_CHANNEL, etc.), and their values
  • to write the sequence of 3-4 functions to set all the parameters for each leaf

With the _fastcreate method you use a single "specific" function for each leaf. It's also leaf-dependent, as before, but simpler.
A user needs:

  • to know only the name of the _fastcreate function (which will be always the same, including the component type in its name, e.g. "cms"), the values of the parameters to set, and their order
  • to write a single function

I understand that the _fastcreate function in C would not be self-explaining regarding its parameters (in python it could have been different), but with proper docs or code templates I believe it would be much simpler for a user to create a leaf and less error-prone than the current method.

Maybe a single leaf-dependent parameter struct could be a way to show to users what they are setting?

However these are just suggestions!

@ogghst
Copy link
Owner

ogghst commented Jan 19, 2022

Thanks for giving me another perspective. We're almost on a phylosophical side, that is what means 'simple' :)

  • for an experienced user, my proposal is simpler because is quicker to write
  • for a newcomer, your proposal is simpler because less error prone and more synthetic

But at the end this entire framework has the ultimate goal to make life easier.

So you won! I'll add it to the TODO list.

User can still approach the 'traditional' way, that is thought also to be ready for higher level automations, and i'll write some _fastcreate functions on the already built leaves.

@ogghst ogghst added this to the 1.0.0 milestone Jan 19, 2022
@ogghst ogghst self-assigned this Jan 19, 2022
@ogghst
Copy link
Owner

ogghst commented Jan 20, 2022

Here's your _fastcreate function, I've put a demo in blink board:

void gn_configure_blink(gn_node_config_handle_t node) {

	//fastcreate call
	gn_leaf_config_handle_t blink = gn_gpio_fastcreate(node, "blink", 2, false, false);

	//creates a timer that fires the led blinking every 5 seconds, using esp_timer API

	esp_timer_handle_t timer_handler;
	esp_timer_create_args_t timer_args = { .callback = &led_blink_callback,
			.arg = blink, .name = "blink_timer" };
	esp_timer_create(&timer_args, &timer_handler);
	esp_timer_start_periodic(timer_handler, 5 * 1000000);

}

Just let me know if you are OK with that, thanks

@adamoferro
Copy link
Collaborator Author

👍👍 very good, thanks

In order to be consistent, probably for a given leaf type all .h/.c should have the same name convention of the leaf function names and configs, as suggested above.
E.g., the .h of the capacity moisture sensor would be renamed from gn_capacitive_moisture_sensor.h to gn_cms.h
Or is there any reason for keeping capacitive_moisture_sensor.h ?

P.S.: there's probably a copy-paste typo at the end of gn_gpio_fastcreate()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants