| Message ID | 20220311073926.78636-3-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | documentation and other minor tweaks | expand |
On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > Both gpiod_request_config and gpiod_line_request contain a number of > lines, but the former has a get_num_offsets accessor, while the latter > has get_num_lines. Make them consistent by switching request_config to > get_num_lines. > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > --- IMO this doesn't make much sense because we still have gpiod_request_config_set_offsets(). So now you have set_offsets but get_lines. At the time of preparing the request_config we're still talking about offsets of lines to request, while once the request has been made, we're talking about requested lines. I would leave it as it is personally. Bart
On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote: > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Both gpiod_request_config and gpiod_line_request contain a number of > > lines, but the former has a get_num_offsets accessor, while the latter > > has get_num_lines. Make them consistent by switching request_config to > > get_num_lines. > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > --- > > IMO this doesn't make much sense because we still have > gpiod_request_config_set_offsets(). So now you have set_offsets but > get_lines. At the time of preparing the request_config we're still > talking about offsets of lines to request, while once the request has > been made, we're talking about requested lines. > Oh FFS we are always talking about lines. Whether you have requested them yet or not is irrelevant. You WANT to request lines. If we had globally unique line names we wouldn't give a rats about the offset. And take another look - you have actually have get_offsets and get_num_lines functions. Offsets is just one of the attributes of the lines, and this approach still works if there is another fields of interest. e.g. values: int gpiod_line_request_set_values_subset(struct gpiod_line_request *request, size_t num_lines, const unsigned int *offsets, const int *values); which you then complain about in patch 4 as I'm writing this.... <sigh>. You could equally argue that one should be num_values. While we are still preparing the configuration, we are preparing the config for LINES, not offsets. Using num_lines is a reminder that you need to provide the offset for each line - the two are inextricably linked. Using num_offsets could be taken to imply that gpiod_request_config_set_offsets() can be called multiple times to add offsets. > I would leave it as it is personally. > I know, I know :-|. Cheers, Kent.
On Tue, Mar 15, 2022 at 12:23 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote: > > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > Both gpiod_request_config and gpiod_line_request contain a number of > > > lines, but the former has a get_num_offsets accessor, while the latter > > > has get_num_lines. Make them consistent by switching request_config to > > > get_num_lines. > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > --- > > > > IMO this doesn't make much sense because we still have > > gpiod_request_config_set_offsets(). So now you have set_offsets but > > get_lines. At the time of preparing the request_config we're still > > talking about offsets of lines to request, while once the request has > > been made, we're talking about requested lines. > > > > Oh FFS we are always talking about lines. Whether you have requested > them yet or not is irrelevant. You WANT to request lines. > If we had globally unique line names we wouldn't give a rats about the > offset. > > And take another look - you have actually have get_offsets and > get_num_lines functions. > > Offsets is just one of the attributes of the lines, and this approach > still works if there is another fields of interest. e.g. values: > > int gpiod_line_request_set_values_subset(struct gpiod_line_request *request, > size_t num_lines, > const unsigned int *offsets, > const int *values); > > which you then complain about in patch 4 as I'm writing this.... <sigh>. > > You could equally argue that one should be num_values. > > While we are still preparing the configuration, we are preparing the > config for LINES, not offsets. Using num_lines is a reminder that you > need to provide the offset for each line - the two are inextricably > linked. Using num_offsets could be taken to imply that > gpiod_request_config_set_offsets() can be called multiple times to add > offsets. > > > I would leave it as it is personally. > > > > I know, I know :-|. > > Cheers, > Kent. I didn't know I would see the whole extend of Kent's wrath after that comment. :) Anyway let me try to defend myself before I wave the white flag and surrender as usual. We're setting VALUES so it's only normal to speak about NUMBER of VALUES. It's like when you have an array of of X that is an attribute of Y, that array still carries a number of X and not Y. This is for patch 4 but this one has another problem. What if we extend this structure to - besides offsets - accept string identifiers for a request? Then we would want to use get_offsets and get_names (or get_ids) and the corresponding get_num_offsets and get_num_names accesors and in this case get_num_lines would become confusing. Bart
On Tue, Mar 15, 2022 at 12:39:56PM +0100, Bartosz Golaszewski wrote: > On Tue, Mar 15, 2022 at 12:23 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > Both gpiod_request_config and gpiod_line_request contain a number of > > > > lines, but the former has a get_num_offsets accessor, while the latter > > > > has get_num_lines. Make them consistent by switching request_config to > > > > get_num_lines. > > > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > --- > > > > > > IMO this doesn't make much sense because we still have > > > gpiod_request_config_set_offsets(). So now you have set_offsets but > > > get_lines. At the time of preparing the request_config we're still > > > talking about offsets of lines to request, while once the request has > > > been made, we're talking about requested lines. > > > > > > > Oh FFS we are always talking about lines. Whether you have requested > > them yet or not is irrelevant. You WANT to request lines. > > If we had globally unique line names we wouldn't give a rats about the > > offset. > > > > And take another look - you have actually have get_offsets and > > get_num_lines functions. > > > > Offsets is just one of the attributes of the lines, and this approach > > still works if there is another fields of interest. e.g. values: > > > > int gpiod_line_request_set_values_subset(struct gpiod_line_request *request, > > size_t num_lines, > > const unsigned int *offsets, > > const int *values); > > > > which you then complain about in patch 4 as I'm writing this.... <sigh>. > > > > You could equally argue that one should be num_values. > > > > While we are still preparing the configuration, we are preparing the > > config for LINES, not offsets. Using num_lines is a reminder that you > > need to provide the offset for each line - the two are inextricably > > linked. Using num_offsets could be taken to imply that > > gpiod_request_config_set_offsets() can be called multiple times to add > > offsets. > > > > > I would leave it as it is personally. > > > > > > > I know, I know :-|. > > > > Cheers, > > Kent. > > I didn't know I would see the whole extend of Kent's wrath after that > comment. :) > We're still way way off the full extent. Though "libgpiod v2 - the Wrath of Kent" does have a certain ring to it. > Anyway let me try to defend myself before I wave the white flag and > surrender as usual. > > We're setting VALUES so it's only normal to speak about NUMBER of VALUES. > But you are happy to call it num_offsets? I'm confused. > It's like when you have an array of of X that is an attribute of Y, > that array still carries a number of X and not Y. > I get that, but in this case the offset is identifier for line. The mapping is 1-1. > This is for patch 4 but this one has another problem. What if we > extend this structure to - besides offsets - accept string identifiers > for a request? Then we would want to use get_offsets and get_names (or > get_ids) and the corresponding get_num_offsets and get_num_names > accesors and in this case get_num_lines would become confusing. > Good luck with that - no matter how you name things. If you allow multiple identifiers then you have to deal with the overlap case. Just don't go there. And what happens to gpiod_line_request_get_offsets where the size of the buffer is determined by gpiod_line_request_get_num_lines()? Much simpler to stick to a single type of identifier for the request. Oh, and them the 1-1 mapping still holds, so you can still use num_lines. No multi-dimensional thinking. Cheers, Kent.
On Tue, Mar 15, 2022 at 12:59 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Mar 15, 2022 at 12:39:56PM +0100, Bartosz Golaszewski wrote: > > On Tue, Mar 15, 2022 at 12:23 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote: > > > > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > Both gpiod_request_config and gpiod_line_request contain a number of > > > > > lines, but the former has a get_num_offsets accessor, while the latter > > > > > has get_num_lines. Make them consistent by switching request_config to > > > > > get_num_lines. > > > > > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > > --- > > > > > > > > IMO this doesn't make much sense because we still have > > > > gpiod_request_config_set_offsets(). So now you have set_offsets but > > > > get_lines. At the time of preparing the request_config we're still > > > > talking about offsets of lines to request, while once the request has > > > > been made, we're talking about requested lines. > > > > > > > > > > Oh FFS we are always talking about lines. Whether you have requested > > > them yet or not is irrelevant. You WANT to request lines. > > > If we had globally unique line names we wouldn't give a rats about the > > > offset. > > > > > > And take another look - you have actually have get_offsets and > > > get_num_lines functions. > > > > > > Offsets is just one of the attributes of the lines, and this approach > > > still works if there is another fields of interest. e.g. values: > > > > > > int gpiod_line_request_set_values_subset(struct gpiod_line_request *request, > > > size_t num_lines, > > > const unsigned int *offsets, > > > const int *values); > > > > > > which you then complain about in patch 4 as I'm writing this.... <sigh>. > > > > > > You could equally argue that one should be num_values. > > > > > > While we are still preparing the configuration, we are preparing the > > > config for LINES, not offsets. Using num_lines is a reminder that you > > > need to provide the offset for each line - the two are inextricably > > > linked. Using num_offsets could be taken to imply that > > > gpiod_request_config_set_offsets() can be called multiple times to add > > > offsets. > > > > > > > I would leave it as it is personally. > > > > > > > > > > I know, I know :-|. > > > > > > Cheers, > > > Kent. > > > > I didn't know I would see the whole extend of Kent's wrath after that > > comment. :) > > > > We're still way way off the full extent. > > Though "libgpiod v2 - the Wrath of Kent" does have a certain ring to it. > Love it, let's make it official. :) > > Anyway let me try to defend myself before I wave the white flag and > > surrender as usual. > > > > We're setting VALUES so it's only normal to speak about NUMBER of VALUES. > > > > But you are happy to call it num_offsets? I'm confused. > Wat? No, the only num_offsets are present in get/set_offsets in request_config. > > It's like when you have an array of of X that is an attribute of Y, > > that array still carries a number of X and not Y. > > > > I get that, but in this case the offset is identifier for line. > The mapping is 1-1. > > > This is for patch 4 but this one has another problem. What if we > > extend this structure to - besides offsets - accept string identifiers > > for a request? Then we would want to use get_offsets and get_names (or > > get_ids) and the corresponding get_num_offsets and get_num_names > > accesors and in this case get_num_lines would become confusing. > > > > Good luck with that - no matter how you name things. > If you allow multiple identifiers then you have to deal with the > overlap case. Just don't go there. > And what happens to gpiod_line_request_get_offsets where the size of > the buffer is determined by gpiod_line_request_get_num_lines()? > The string identifiers would be translated to offsets at some point. Here it would make sense to retrieve the number of lines ACTUALLY requested and get their OFFSETS of which there are NUM_LINES. > Much simpler to stick to a single type of identifier for the request. > Oh, and them the 1-1 mapping still holds, so you can still use num_lines. > No multi-dimensional thinking. > I don't see a problem with current naming. You set offsets -> num_offsets, values -> num_values. Also: unlike function names this is not even part of ABI. We can even name it `n`, `nelem`, `num_elem` everywhere for all I care. Bart
On Tue, Mar 15, 2022 at 02:43:28PM +0100, Bartosz Golaszewski wrote: > On Tue, Mar 15, 2022 at 12:59 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Mar 15, 2022 at 12:39:56PM +0100, Bartosz Golaszewski wrote: > > > On Tue, Mar 15, 2022 at 12:23 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Tue, Mar 15, 2022 at 11:52:21AM +0100, Bartosz Golaszewski wrote: > > > > > On Fri, Mar 11, 2022 at 8:40 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > Both gpiod_request_config and gpiod_line_request contain a number of > > > > > > lines, but the former has a get_num_offsets accessor, while the latter > > > > > > has get_num_lines. Make them consistent by switching request_config to > > > > > > get_num_lines. > > > > > > > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > > > --- > > > > > > > > > > IMO this doesn't make much sense because we still have > > > > > gpiod_request_config_set_offsets(). So now you have set_offsets but > > > > > get_lines. At the time of preparing the request_config we're still > > > > > talking about offsets of lines to request, while once the request has > > > > > been made, we're talking about requested lines. > > > > > > > > > > > > > Oh FFS we are always talking about lines. Whether you have requested > > > > them yet or not is irrelevant. You WANT to request lines. > > > > If we had globally unique line names we wouldn't give a rats about the > > > > offset. > > > > > > > > And take another look - you have actually have get_offsets and > > > > get_num_lines functions. > > > > > > > > Offsets is just one of the attributes of the lines, and this approach > > > > still works if there is another fields of interest. e.g. values: > > > > > > > > int gpiod_line_request_set_values_subset(struct gpiod_line_request *request, > > > > size_t num_lines, > > > > const unsigned int *offsets, > > > > const int *values); > > > > > > > > which you then complain about in patch 4 as I'm writing this.... <sigh>. > > > > > > > > You could equally argue that one should be num_values. > > > > > > > > While we are still preparing the configuration, we are preparing the > > > > config for LINES, not offsets. Using num_lines is a reminder that you > > > > need to provide the offset for each line - the two are inextricably > > > > linked. Using num_offsets could be taken to imply that > > > > gpiod_request_config_set_offsets() can be called multiple times to add > > > > offsets. > > > > > > > > > I would leave it as it is personally. > > > > > > > > > > > > > I know, I know :-|. > > > > > > > > Cheers, > > > > Kent. > > > > > > I didn't know I would see the whole extend of Kent's wrath after that > > > comment. :) > > > > > > > We're still way way off the full extent. > > > > Though "libgpiod v2 - the Wrath of Kent" does have a certain ring to it. > > > > Love it, let's make it official. :) > Maybe not - one of the good guys dies at the end of that one, as does the eponymous character :-(. > > > Anyway let me try to defend myself before I wave the white flag and > > > surrender as usual. > > > > > > We're setting VALUES so it's only normal to speak about NUMBER of VALUES. > > > > > > > But you are happy to call it num_offsets? I'm confused. > > > > Wat? No, the only num_offsets are present in get/set_offsets in request_config. > My bad - you were being abstract and on first reading I took it literally. My perspective is that you are setting the ATTRs on a NUMBER of OBJECTS, so looking at it with the scope of the config, not the individual function. But I see your point. > > > It's like when you have an array of of X that is an attribute of Y, > > > that array still carries a number of X and not Y. > > > > > > > I get that, but in this case the offset is identifier for line. > > The mapping is 1-1. > > > > > This is for patch 4 but this one has another problem. What if we > > > extend this structure to - besides offsets - accept string identifiers > > > for a request? Then we would want to use get_offsets and get_names (or > > > get_ids) and the corresponding get_num_offsets and get_num_names > > > accesors and in this case get_num_lines would become confusing. > > > > > > > Good luck with that - no matter how you name things. > > If you allow multiple identifiers then you have to deal with the > > overlap case. Just don't go there. > > And what happens to gpiod_line_request_get_offsets where the size of > > the buffer is determined by gpiod_line_request_get_num_lines()? > > > > The string identifiers would be translated to offsets at some point. > Here it would make sense to retrieve the number of lines ACTUALLY > requested and get their OFFSETS of which there are NUM_LINES. > For the tool prototyping I've been doing I went with stringified id -> line, with the stringified id mapped to line depending on the other command line options, so it may be a name or an offset, depending. Behind the scenes the line is always (chip,offset). But that is really a higher level of abstraction that should be built over libgpiod core, not builtin to it. Unless it were also added to the uAPI. > > Much simpler to stick to a single type of identifier for the request. > > Oh, and them the 1-1 mapping still holds, so you can still use num_lines. > > No multi-dimensional thinking. > > > > I don't see a problem with current naming. You set offsets -> > num_offsets, values -> num_values. Also: unlike function names this is > not even part of ABI. We can even name it `n`, `nelem`, `num_elem` > everywhere for all I care. > A generic might be the way to go for the (offset,value) pairs split over arrays case. Cheers, Kent.
diff --git a/include/gpiod.h b/include/gpiod.h index e6a4645..8fb70ee 100644 --- a/include/gpiod.h +++ b/include/gpiod.h @@ -1094,12 +1094,12 @@ void gpiod_request_config_set_offsets(struct gpiod_request_config *config, const unsigned int *offsets); /** - * @brief Get the number of offsets configured in this request config. + * @brief Get the number of lines configured in this request config. * @param config Request config object. - * @return Number of line offsets in this request config. + * @return Number of lines to be requested by this config. */ size_t -gpiod_request_config_get_num_offsets(struct gpiod_request_config *config); +gpiod_request_config_get_num_lines(struct gpiod_request_config *config); /** * @brief Get the hardware offsets of lines in this request config. diff --git a/lib/request-config.c b/lib/request-config.c index abcca58..d9ecc8e 100644 --- a/lib/request-config.c +++ b/lib/request-config.c @@ -13,7 +13,7 @@ struct gpiod_request_config { char consumer[GPIO_MAX_NAME_SIZE]; unsigned int offsets[GPIO_V2_LINES_MAX]; - size_t num_offsets; + size_t num_lines; size_t event_buffer_size; }; @@ -54,22 +54,22 @@ gpiod_request_config_get_consumer(struct gpiod_request_config *config) GPIOD_API void gpiod_request_config_set_offsets(struct gpiod_request_config *config, - size_t num_offsets, + size_t num_lines, const unsigned int *offsets) { size_t i; - config->num_offsets = num_offsets > GPIO_V2_LINES_MAX ? - GPIO_V2_LINES_MAX : num_offsets; + config->num_lines = num_lines > GPIO_V2_LINES_MAX ? + GPIO_V2_LINES_MAX : num_lines; - for (i = 0; i < config->num_offsets; i++) + for (i = 0; i < config->num_lines; i++) config->offsets[i] = offsets[i]; } GPIOD_API size_t -gpiod_request_config_get_num_offsets(struct gpiod_request_config *config) +gpiod_request_config_get_num_lines(struct gpiod_request_config *config) { - return config->num_offsets; + return config->num_lines; } GPIOD_API void @@ -77,7 +77,7 @@ gpiod_request_config_get_offsets(struct gpiod_request_config *config, unsigned int *offsets) { memcpy(offsets, config->offsets, - sizeof(*offsets) * config->num_offsets); + sizeof(*offsets) * config->num_lines); } GPIOD_API void @@ -98,15 +98,15 @@ int gpiod_request_config_to_kernel(struct gpiod_request_config *config, { size_t i; - if (config->num_offsets == 0) { + if (config->num_lines == 0) { errno = EINVAL; return -1; } - for (i = 0; i < config->num_offsets; i++) + for (i = 0; i < config->num_lines; i++) reqbuf->offsets[i] = config->offsets[i]; - reqbuf->num_lines = config->num_offsets; + reqbuf->num_lines = config->num_lines; strcpy(reqbuf->consumer, config->consumer); reqbuf->event_buffer_size = config->event_buffer_size; diff --git a/tests/tests-line-request.c b/tests/tests-line-request.c index 8ef391d..eb3adee 100644 --- a/tests/tests-line-request.c +++ b/tests/tests-line-request.c @@ -21,7 +21,7 @@ GPIOD_TEST_CASE(request_fails_with_no_offsets) req_cfg = gpiod_test_create_request_config_or_fail(); line_cfg = gpiod_test_create_line_config_or_fail(); - g_assert_cmpuint(gpiod_request_config_get_num_offsets(req_cfg), ==, 0); + g_assert_cmpuint(gpiod_request_config_get_num_lines(req_cfg), ==, 0); chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim)); request = gpiod_chip_request_lines(chip, req_cfg, line_cfg); diff --git a/tests/tests-request-config.c b/tests/tests-request-config.c index becb414..136fe07 100644 --- a/tests/tests-request-config.c +++ b/tests/tests-request-config.c @@ -16,7 +16,7 @@ GPIOD_TEST_CASE(default_config) config = gpiod_test_create_request_config_or_fail(); g_assert_null(gpiod_request_config_get_consumer(config)); - g_assert_cmpuint(gpiod_request_config_get_num_offsets(config), ==, 0); + g_assert_cmpuint(gpiod_request_config_get_num_lines(config), ==, 0); g_assert_cmpuint(gpiod_request_config_get_event_buffer_size(config), ==, 0); } @@ -42,7 +42,7 @@ GPIOD_TEST_CASE(offsets) config = gpiod_test_create_request_config_or_fail(); gpiod_request_config_set_offsets(config, 4, offsets); - g_assert_cmpuint(gpiod_request_config_get_num_offsets(config), ==, 4); + g_assert_cmpuint(gpiod_request_config_get_num_lines(config), ==, 4); memset(read_back, 0, sizeof(read_back)); gpiod_request_config_get_offsets(config, read_back); for (i = 0; i < 4; i++) @@ -71,11 +71,11 @@ GPIOD_TEST_CASE(max_offsets) config = gpiod_test_create_request_config_or_fail(); gpiod_request_config_set_offsets(config, 64, offsets_good); - g_assert_cmpuint(gpiod_request_config_get_num_offsets(config), ==, 64); + g_assert_cmpuint(gpiod_request_config_get_num_lines(config), ==, 64); gpiod_request_config_set_offsets(config, 65, offsets_bad); /* Should get truncated. */ - g_assert_cmpuint(gpiod_request_config_get_num_offsets(config), ==, 64); + g_assert_cmpuint(gpiod_request_config_get_num_lines(config), ==, 64); } GPIOD_TEST_CASE(event_buffer_size)
Both gpiod_request_config and gpiod_line_request contain a number of lines, but the former has a get_num_offsets accessor, while the latter has get_num_lines. Make them consistent by switching request_config to get_num_lines. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- include/gpiod.h | 6 +++--- lib/request-config.c | 22 +++++++++++----------- tests/tests-line-request.c | 2 +- tests/tests-request-config.c | 8 ++++---- 4 files changed, 19 insertions(+), 19 deletions(-)