| Message ID | 20210410145157.30718-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | first draft of libgpiod v2.0 API | expand |
On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote: > This series introduces an entirely reworked API for the core C part of > libgpiod. It's not fully functional as the bindings are not modified, > and starting from patch 5, even the tests stop working. This is on > purpose as I'd like to reach an agreement on the interface before > spending time on reworking the tests. > > My plan for the development of v2.0 is to keep the changes in a separate > branch until all bits & pieces are complete and then rearrange them into > a bisectable series that will be merged into the master branch. > > A couple points regarding the design of the new API: > - all objects have become opaque and can only be accessed using dedicated > functions > - line objects as well as bulk containers have been removed > - line requests are now configured using three types of structures: attributes, > line config and request config structures, which follows the kernel API > - line request handles have now a lifetime separate from the parent chips to > leverage the separation of chip and request file descriptors > - line events are now opaque but they can be stored in a dedicated container > so that memory allocations are not necessary everytime an event is read from > the kernel > - the library code has been split into several compilation units for easier > maintenance > > The new API is completely documented in include/gpiod.h doxygen comments. > > Please let me know what you think. I am aware that these patches are huge and > difficult to review but I'm really only expecting a general API review - all > other implementation details can be improved later. > In that vein, I'll lump my comments here, rather than scattering them throughout the patches... Overall it feels much tighter and clearer than the old API, though that could just be personal bias, as it is also closer to the new uAPI. I find the ownership and lifetime of objects confusing. I presume you want to use the reference counting to simplify the object lifecycle, but that just formalises the possibility that objects are shared - further confusing the ownership issue. e.g. gpiod_line_config_add_attribute() - who owns the attr after the call? What happens if it is subsequently modified? Is the attr copied or does ownership pass to the config? As written it is neither - the attr becomes shared. How is that preferable? Similarly gpiod_line_get_consumer() - who owns the returned pointer and what is it's lifetime? I would prefer the opaque objects to be able to be free()d normally, and for any calls that involve a change of ownership to explicitly document that fact. For objects that require additional setup/cleanup, try to make use of open()/close() or request()/release() function name pairings. So gpiod_chip would have open()/close(), gpiod_request_handle would have request()/release(), and the others would all be new()/free()d. Conceptually the config for each line can be considered to be independent - the uAPI encoding using attributes and masks is only there to keep the uAPI structs to a manageable size. At this level you can model it as config per line, and only map between the uAPI structures when calling the ioctl()s. So I would drop the gpiod_line_attr, and instead provide accessors and mutators on the gpiod_line_config for each attr type. That removes the whole lifecycle issue for attributes, and allows you to provide a simpler abstraction of the config than that provided in the uAPI. For the mutators there can be two flavours, one that sets the config for the whole set, and another that accepts a subset of applicable lines. Accessors would provide the config attr for a particular line. Both accessor and mutator would use chip offsets to identify the lines. Not sure I like merging the direction and edge into the request_type. I would tend to keep those separate, with set_direction and set_edge_detection functions, with the latter forcing input direction. I would rename gpiod_request_config to gpiod_request_options. Config is long lived and can be modified, whereas options are one-off. And it would reduce any chance of confusion with gpiod_line_config. gpiod_line_mask should highlight that the bits correspond to lines on the request, in the order provided to gpiod_request_config_set_offsets(), not line offsets on the chip. Perhaps the gpiod_request_config or the gpiod_request_handle should provide the mask functions for a given a qchip offset, rather than require the user to track the mapping? Then the gpiod_line_mask can be opaque as well. I would rename gpiod_request_handle to gpiod_line_request. And request.c to options.c, and handle.c to request.c. gpiod_line_attr_set_debounce() can use a zero period to identify disabling debounce, so the debounce flag is redundant. Cheers, Kent.
Hi Kent! Thanks a lot for looking at this. On Wed, Apr 14, 2021 at 4:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote: > > This series introduces an entirely reworked API for the core C part of > > libgpiod. It's not fully functional as the bindings are not modified, > > and starting from patch 5, even the tests stop working. This is on > > purpose as I'd like to reach an agreement on the interface before > > spending time on reworking the tests. > > > > My plan for the development of v2.0 is to keep the changes in a separate > > branch until all bits & pieces are complete and then rearrange them into > > a bisectable series that will be merged into the master branch. > > > > A couple points regarding the design of the new API: > > - all objects have become opaque and can only be accessed using dedicated > > functions > > - line objects as well as bulk containers have been removed > > - line requests are now configured using three types of structures: attributes, > > line config and request config structures, which follows the kernel API > > - line request handles have now a lifetime separate from the parent chips to > > leverage the separation of chip and request file descriptors > > - line events are now opaque but they can be stored in a dedicated container > > so that memory allocations are not necessary everytime an event is read from > > the kernel > > - the library code has been split into several compilation units for easier > > maintenance > > > > The new API is completely documented in include/gpiod.h doxygen comments. > > > > Please let me know what you think. I am aware that these patches are huge and > > difficult to review but I'm really only expecting a general API review - all > > other implementation details can be improved later. > > > Just to clarify why I did certain things the way I did: there are certain semi-standardized guidelines to creating low-level C libraries for linux user-space interfaces. They are gathered in a sample project called libabc[1][2][3]. Most plumbing-layer libraries follow these guidelines more or less rigorously. Many low-level programmers will make certain assumptions based on their previous experiences when working with libraries and it's good to be as little confusing as possible. > In that vein, I'll lump my comments here, rather than scattering them > throughout the patches... > > Overall it feels much tighter and clearer than the old API, though that > could just be personal bias, as it is also closer to the new uAPI. > > I find the ownership and lifetime of objects confusing. I presume you > want to use the reference counting to simplify the object lifecycle, but > that just formalises the possibility that objects are shared - further > confusing the ownership issue. Interesting because in my mind reference counting actually simplifies the life-time of objects - everytime someone (the caller or another object) stores the address of the object, the reference count is increased. Reference counting is a de-facto standard in C libraries. Look at libkmod, libudev, libpulse, all the systemd libraries, the whole of GLib. Reference counting makes higher-level programming easier. An object lives as long as there's at least one user. In C++ bindings the objects were refcounted using shared_ptr which causes some issues. Now with refcounting implemented at the C library level, we'll be able to use the PImpl pattern with unique_ptr and still keep the objects copyable. To summarize: I don't see many disadvantages to using reference counting other than the fact that my documentation is not yet very detailed and lacks some general information about how reference counting works in libgpiod and needs some clarification. > e.g. gpiod_line_config_add_attribute() > - who owns the attr after the call? What happens if it is subsequently > modified? Is the attr copied or does ownership pass to the config? > As written it is neither - the attr becomes shared. How is that > preferable? Yes, it becomes shared. Its reference count is now 2. If the user modifies it, the config will use a modified value at the time of the request. To give you a real-life example of such behavior: in GLib where all GObjects are reference counted, if you pass a reference to a GFile to another user, then set its (GFile's) attribute, they'll be changed for the user sharing the object too. Same with how Python or Java handle references, except you don't need to manage them yourself. > Similarly gpiod_line_get_consumer() - who owns the returned pointer and > what is it's lifetime? Since simple strings can't be refcounted and this function returns a pointer to a constant string, the life-time of the string is obviously tied to that of the line_info object. The doc should probably say: Returns a pointer to the string stored in the line_info object. > I would prefer the opaque objects to be able to be free()d normally, and > for any calls that involve a change of ownership to explicitly document > that fact. > For objects that require additional setup/cleanup, try to make use of > open()/close() or request()/release() function name pairings. > So gpiod_chip would have open()/close(), gpiod_request_handle would have > request()/release(), and the others would all be new()/free()d. > This makes you use a different set of resource management functions depending on the object. Additionally some objects can't be created - only retrieved from other objects. Having a simple ref/unref pair for all opaque data types looks more elegant to me. Another real-life example from yesterday: I'm working on a small library to go with the new gpio-sim module (meant as a replacement for libgpio-mockup) and when I tried to use libmount (to verify if configfs is mounted and where) I was super confused by its resource management which is similar to what you suggest but also some resources need to be freed and some transfer their ownership to other objects (without reference counting) and while it's all documented and you can find info on that eventually, I still spent time debugging strange double-free and other corruptions before I got things right. I find libudev and libkmod with their consistent reference counting much more instinctive to use. > Conceptually the config for each line can be considered to be independent > - the uAPI encoding using attributes and masks is only there to keep the > uAPI structs to a manageable size. > At this level you can model it as config per line, and only map > between the uAPI structures when calling the ioctl()s. > So I would drop the gpiod_line_attr, and instead provide accessors and > mutators on the gpiod_line_config for each attr type. > That removes the whole lifecycle issue for attributes, and allows you to > provide a simpler abstraction of the config than that provided in the > uAPI. > For the mutators there can be two flavours, one that sets the config for > the whole set, and another that accepts a subset of applicable lines. > Accessors would provide the config attr for a particular line. > Both accessor and mutator would use chip offsets to identify the lines. > I had exactly this in mind initially but couldn't find an elegant interface. There are two options I see: gpiod_line_config_set_drive(cfg, drive); gpiod_line_config_set_drive_index(cfg, drive, index); or: gpiod_line_config_set_drive(cfg, drive); unsigned int indices[] = { 0, 3, 4, 6 }; gpiod_line_config_set_drive_index(cfg, drive, indices); The former would require a separate function call for each line, the former needs a lot of boilerplate code because we'll usually have to alloc & fill the indices array. Eventually I decided to go with attributes but if you have some ideas/suggestions for that, they're welcome! > Not sure I like merging the direction and edge into the request_type. > I would tend to keep those separate, with set_direction and > set_edge_detection functions, with the latter forcing input direction. > If there's no way we can use output with edge detection, why even bother with allowing the user to try to set output mode for edge_detection or vice-versa? It's just more error checking for no benefit, right? The user will still find out that the line is in input mode with the line_info. > I would rename gpiod_request_config to gpiod_request_options. Config > is long lived and can be modified, whereas options are one-off. > And it would reduce any chance of confusion with gpiod_line_config. > Not sure about this one. With the current naming, it's being made clear that we have two separate things to configure, the line(s) and the request itself. Naming both "_config" makes a clear distinction. Also: the request config can be reused too for separate requests. > gpiod_line_mask should highlight that the bits correspond to lines on > the request, in the order provided to gpiod_request_config_set_offsets(), > not line offsets on the chip. Perhaps the gpiod_request_config or the > gpiod_request_handle should provide the mask functions for a given a > qchip offset, rather than require the user to track the mapping? > Then the gpiod_line_mask can be opaque as well. > Yes, this is one of those things I struggled with. I wasn't sure which one's better because if the user wants to request 64 lines, then set a separate config for 20 of them, it's a lot of function calls if we allow to set line config by offset of a single line, or we get a complicated API with allowing arrays of offsets. Agreed for the line map functions, it's probably the most confusing element of this new API. I'll come up with something. > I would rename gpiod_request_handle to gpiod_line_request. > And request.c to options.c, and handle.c to request.c. > > gpiod_line_attr_set_debounce() can use a zero period to identify > disabling debounce, so the debounce flag is redundant. > Sounds good. Bart > Cheers, > Kent. [1] http://0pointer.de/blog/projects/libabc.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git [3] https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git/plain/README
On Fri, Apr 16, 2021 at 11:36:08AM +0200, Bartosz Golaszewski wrote: > Hi Kent! > > Thanks a lot for looking at this. > > On Wed, Apr 14, 2021 at 4:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote: > > > This series introduces an entirely reworked API for the core C part of > > > libgpiod. It's not fully functional as the bindings are not modified, > > > and starting from patch 5, even the tests stop working. This is on > > > purpose as I'd like to reach an agreement on the interface before > > > spending time on reworking the tests. > > > > > > My plan for the development of v2.0 is to keep the changes in a separate > > > branch until all bits & pieces are complete and then rearrange them into > > > a bisectable series that will be merged into the master branch. > > > > > > A couple points regarding the design of the new API: > > > - all objects have become opaque and can only be accessed using dedicated > > > functions > > > - line objects as well as bulk containers have been removed > > > - line requests are now configured using three types of structures: attributes, > > > line config and request config structures, which follows the kernel API > > > - line request handles have now a lifetime separate from the parent chips to > > > leverage the separation of chip and request file descriptors > > > - line events are now opaque but they can be stored in a dedicated container > > > so that memory allocations are not necessary everytime an event is read from > > > the kernel > > > - the library code has been split into several compilation units for easier > > > maintenance > > > > > > The new API is completely documented in include/gpiod.h doxygen comments. > > > > > > Please let me know what you think. I am aware that these patches are huge and > > > difficult to review but I'm really only expecting a general API review - all > > > other implementation details can be improved later. > > > > > > > Just to clarify why I did certain things the way I did: there are > certain semi-standardized guidelines to creating low-level C libraries > for linux user-space interfaces. They are gathered in a sample project > called libabc[1][2][3]. Most plumbing-layer libraries follow these > guidelines more or less rigorously. Many low-level programmers will > make certain assumptions based on their previous experiences when > working with libraries and it's good to be as little confusing as > possible. > Fair enough, though I don't see anything in libabc that conflicts with my suggestions, nor that require some of your design choices. > > In that vein, I'll lump my comments here, rather than scattering them > > throughout the patches... > > > > Overall it feels much tighter and clearer than the old API, though that > > could just be personal bias, as it is also closer to the new uAPI. > > > > I find the ownership and lifetime of objects confusing. I presume you > > want to use the reference counting to simplify the object lifecycle, but > > that just formalises the possibility that objects are shared - further > > confusing the ownership issue. > > Interesting because in my mind reference counting actually simplifies > the life-time of objects - everytime someone (the caller or another > object) stores the address of the object, the reference count is > increased. > > Reference counting is a de-facto standard in C libraries. Look at > libkmod, libudev, libpulse, all the systemd libraries, the whole of > GLib. > I don't doubt that reference counting is out there, I question whether it is necessary here. In my mind reference counting is a tool to be used when you need to share objects and it isn't clear who is responsible for freeing them. I don't see any need for sharing here at all - for all objects (excluding the gpiod_line_attr that I would remove) it is pretty clear who is responsible for them at all times. > Reference counting makes higher-level programming easier. An object > lives as long as there's at least one user. In C++ bindings the > objects were refcounted using shared_ptr which causes some issues. Now > with refcounting implemented at the C library level, we'll be able to > use the PImpl pattern with unique_ptr and still keep the objects > copyable. > That still sounds to me like you are sharing objects that shouldn't be shared. Or copied. > To summarize: I don't see many disadvantages to using reference > counting other than the fact that my documentation is not yet very > detailed and lacks some general information about how reference > counting works in libgpiod and needs some clarification. > I understand your argument - which is exactly what I had presumed. But I still find the idea of having _unref() acting as the universal free function to be distasteful. > > e.g. gpiod_line_config_add_attribute() > > - who owns the attr after the call? What happens if it is subsequently > > modified? Is the attr copied or does ownership pass to the config? > > As written it is neither - the attr becomes shared. How is that > > preferable? > > Yes, it becomes shared. Its reference count is now 2. If the user > modifies it, the config will use a modified value at the time of the > request. To give you a real-life example of such behavior: in GLib > where all GObjects are reference counted, if you pass a reference to a > GFile to another user, then set its (GFile's) attribute, they'll be > changed for the user sharing the object too. Same with how Python or > Java handle references, except you don't need to manage them yourself. > This is all mute if you drop gpiod_line_attr, but I still don't see why sharing is preferable in this case. It feels like the worst of both worlds - I need to alloc an attr for every add, and I need to unref them as well. > > Similarly gpiod_line_get_consumer() - who owns the returned pointer and > > what is it's lifetime? > > Since simple strings can't be refcounted and this function returns a > pointer to a constant string, the life-time of the string is obviously > tied to that of the line_info object. The doc should probably say: > Returns a pointer to the string stored in the line_info object. > I would dispute the "obviously", particularly since you seem to expect everything else to be unref()ed. > > I would prefer the opaque objects to be able to be free()d normally, and > > for any calls that involve a change of ownership to explicitly document > > that fact. > > For objects that require additional setup/cleanup, try to make use of > > open()/close() or request()/release() function name pairings. > > So gpiod_chip would have open()/close(), gpiod_request_handle would have > > request()/release(), and the others would all be new()/free()d. > > > > This makes you use a different set of resource management functions > depending on the object. Additionally some objects can't be created - > only retrieved from other objects. > Yup, different objects have different lifecycles - even your way. > Having a simple ref/unref pair for all opaque data types looks more > elegant to me. Another real-life example from yesterday: I'm working > on a small library to go with the new gpio-sim module (meant as a > replacement for libgpio-mockup) and when I tried to use libmount (to > verify if configfs is mounted and where) I was super confused by its > resource management which is similar to what you suggest but also some > resources need to be freed and some transfer their ownership to other > objects (without reference counting) and while it's all documented and > you can find info on that eventually, I still spent time debugging > strange double-free and other corruptions before I got things right. I > find libudev and libkmod with their consistent reference counting much > more instinctive to use. > I find dealing with repeated frees preferable to dealing with issues related to unexpected side-effects when ownership is unclear. > > Conceptually the config for each line can be considered to be independent > > - the uAPI encoding using attributes and masks is only there to keep the > > uAPI structs to a manageable size. > > At this level you can model it as config per line, and only map > > between the uAPI structures when calling the ioctl()s. > > So I would drop the gpiod_line_attr, and instead provide accessors and > > mutators on the gpiod_line_config for each attr type. > > That removes the whole lifecycle issue for attributes, and allows you to > > provide a simpler abstraction of the config than that provided in the > > uAPI. > > For the mutators there can be two flavours, one that sets the config for > > the whole set, and another that accepts a subset of applicable lines. > > Accessors would provide the config attr for a particular line. > > Both accessor and mutator would use chip offsets to identify the lines. > > > > I had exactly this in mind initially but couldn't find an elegant interface. > > There are two options I see: > > gpiod_line_config_set_drive(cfg, drive); > gpiod_line_config_set_drive_index(cfg, drive, index); > > or: > > gpiod_line_config_set_drive(cfg, drive); > unsigned int indices[] = { 0, 3, 4, 6 }; > gpiod_line_config_set_drive_index(cfg, drive, indices); > > The former would require a separate function call for each line, the > former needs a lot of boilerplate code because we'll usually have to > alloc & fill the indices array. Eventually I decided to go with > attributes but if you have some ideas/suggestions for that, they're > welcome! > I generally avoid variadic functions, but this might be an application where they make sense. i.e. make your indices variadic. Oh, and make them offsets ;-). > > Not sure I like merging the direction and edge into the request_type. > > I would tend to keep those separate, with set_direction and > > set_edge_detection functions, with the latter forcing input direction. > > > > If there's no way we can use output with edge detection, why even > bother with allowing the user to try to set output mode for > edge_detection or vice-versa? It's just more error checking for no > benefit, right? The user will still find out that the line is in input > mode with the line_info. > You are adding another concept for the user to grok - the request_type. They will need to understand direction and edge detection anyway, why add a new concept to the mix? Oh, and wrt error checking, in my Go library, and as I suggest above, setting edge detection overrides any previous direction setting and forces input. Similarly setting output direction cancels any edge detection. So the config presented to the kernel will always be consistent and never trigger an EINVAL. And that is all documented in the corresponding config functions. Similarly the drive settings. The library policy is last-in-wins, which it can be as there are distinct calls for each attribute, whereas at the uAPI they are merged into flags. > > I would rename gpiod_request_config to gpiod_request_options. Config > > is long lived and can be modified, whereas options are one-off. > > And it would reduce any chance of confusion with gpiod_line_config. > > > > Not sure about this one. With the current naming, it's being made > clear that we have two separate things to configure, the line(s) and > the request itself. Naming both "_config" makes a clear distinction. > Also: the request config can be reused too for separate requests. > I meant one-off wrt a single request. i.e. changing the options after the lines have been requested can never impact the request, whereas a changed config can be applied with the set_config(). > > gpiod_line_mask should highlight that the bits correspond to lines on > > the request, in the order provided to gpiod_request_config_set_offsets(), > > not line offsets on the chip. Perhaps the gpiod_request_config or the > > gpiod_request_handle should provide the mask functions for a given a > > qchip offset, rather than require the user to track the mapping? > > Then the gpiod_line_mask can be opaque as well. > > > > Yes, this is one of those things I struggled with. I wasn't sure which > one's better because if the user wants to request 64 lines, then set a > separate config for 20 of them, it's a lot of function calls if we > allow to set line config by offset of a single line, or we get a > complicated API with allowing arrays of offsets. > > Agreed for the line map functions, it's probably the most confusing > element of this new API. I'll come up with something. > Ideally I would like to avoid the gpiod_line_mask totally, and always deal in chip offsets. The API should be simplest for the most common use cases. Those far corner cases can be a pain to use, but must still be possible. I would put the priority on handling the most common cases: A. all lines B. one line C. a subset of lines I would expect A to be the most common by far - that was all that the previous API supported, then B and then C. I had suggested providing A and C, with B being a special case of C. But I could live with C being implemented as multiple calls to B. A variadic function could manage all three, with no offsets meaning all lines? Though in practice I would probably provide distinct functions for A and B, and reserve the variadic for C. Cheers, Kent.
On Sat, Apr 17, 2021 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Apr 16, 2021 at 11:36:08AM +0200, Bartosz Golaszewski wrote: > > Hi Kent! > > > > Thanks a lot for looking at this. > > > > On Wed, Apr 14, 2021 at 4:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote: > > > > This series introduces an entirely reworked API for the core C part of > > > > libgpiod. It's not fully functional as the bindings are not modified, > > > > and starting from patch 5, even the tests stop working. This is on > > > > purpose as I'd like to reach an agreement on the interface before > > > > spending time on reworking the tests. > > > > > > > > My plan for the development of v2.0 is to keep the changes in a separate > > > > branch until all bits & pieces are complete and then rearrange them into > > > > a bisectable series that will be merged into the master branch. > > > > > > > > A couple points regarding the design of the new API: > > > > - all objects have become opaque and can only be accessed using dedicated > > > > functions > > > > - line objects as well as bulk containers have been removed > > > > - line requests are now configured using three types of structures: attributes, > > > > line config and request config structures, which follows the kernel API > > > > - line request handles have now a lifetime separate from the parent chips to > > > > leverage the separation of chip and request file descriptors > > > > - line events are now opaque but they can be stored in a dedicated container > > > > so that memory allocations are not necessary everytime an event is read from > > > > the kernel > > > > - the library code has been split into several compilation units for easier > > > > maintenance > > > > > > > > The new API is completely documented in include/gpiod.h doxygen comments. > > > > > > > > Please let me know what you think. I am aware that these patches are huge and > > > > difficult to review but I'm really only expecting a general API review - all > > > > other implementation details can be improved later. > > > > > > > > > > > Just to clarify why I did certain things the way I did: there are > > certain semi-standardized guidelines to creating low-level C libraries > > for linux user-space interfaces. They are gathered in a sample project > > called libabc[1][2][3]. Most plumbing-layer libraries follow these > > guidelines more or less rigorously. Many low-level programmers will > > make certain assumptions based on their previous experiences when > > working with libraries and it's good to be as little confusing as > > possible. > > > > Fair enough, though I don't see anything in libabc that conflicts with > my suggestions, nor that require some of your design choices. > > > > In that vein, I'll lump my comments here, rather than scattering them > > > throughout the patches... > > > > > > Overall it feels much tighter and clearer than the old API, though that > > > could just be personal bias, as it is also closer to the new uAPI. > > > > > > I find the ownership and lifetime of objects confusing. I presume you > > > want to use the reference counting to simplify the object lifecycle, but > > > that just formalises the possibility that objects are shared - further > > > confusing the ownership issue. > > > > Interesting because in my mind reference counting actually simplifies > > the life-time of objects - everytime someone (the caller or another > > object) stores the address of the object, the reference count is > > increased. > > > > Reference counting is a de-facto standard in C libraries. Look at > > libkmod, libudev, libpulse, all the systemd libraries, the whole of > > GLib. > > > > I don't doubt that reference counting is out there, I question whether > it is necessary here. > > In my mind reference counting is a tool to be used when you need to > share objects and it isn't clear who is responsible for freeing > them. I don't see any need for sharing here at all - for all objects > (excluding the gpiod_line_attr that I would remove) it is pretty clear > who is responsible for them at all times. > > > Reference counting makes higher-level programming easier. An object > > lives as long as there's at least one user. In C++ bindings the > > objects were refcounted using shared_ptr which causes some issues. Now > > with refcounting implemented at the C library level, we'll be able to > > use the PImpl pattern with unique_ptr and still keep the objects > > copyable. > > > > That still sounds to me like you are sharing objects that shouldn't be > shared. Or copied. > > > To summarize: I don't see many disadvantages to using reference > > counting other than the fact that my documentation is not yet very > > detailed and lacks some general information about how reference > > counting works in libgpiod and needs some clarification. > > > > I understand your argument - which is exactly what I had presumed. > But I still find the idea of having _unref() acting as the universal free > function to be distasteful. > > > > e.g. gpiod_line_config_add_attribute() > > > - who owns the attr after the call? What happens if it is subsequently > > > modified? Is the attr copied or does ownership pass to the config? > > > As written it is neither - the attr becomes shared. How is that > > > preferable? > > > > Yes, it becomes shared. Its reference count is now 2. If the user > > modifies it, the config will use a modified value at the time of the > > request. To give you a real-life example of such behavior: in GLib > > where all GObjects are reference counted, if you pass a reference to a > > GFile to another user, then set its (GFile's) attribute, they'll be > > changed for the user sharing the object too. Same with how Python or > > Java handle references, except you don't need to manage them yourself. > > > > This is all mute if you drop gpiod_line_attr, but I still don't see why > sharing is preferable in this case. It feels like the worst of both > worlds - I need to alloc an attr for every add, and I need to unref them > as well. > > > > Similarly gpiod_line_get_consumer() - who owns the returned pointer and > > > what is it's lifetime? > > > > Since simple strings can't be refcounted and this function returns a > > pointer to a constant string, the life-time of the string is obviously > > tied to that of the line_info object. The doc should probably say: > > Returns a pointer to the string stored in the line_info object. > > > > I would dispute the "obviously", particularly since you seem to expect > everything else to be unref()ed. > No, the ref/unref mechanism is customary for opaque structures. Const string pointers are always stored in some other object because passing const char * to free will result in a warning. In libc all functions that return a malloc()'ed string return char *. > > > I would prefer the opaque objects to be able to be free()d normally, and > > > for any calls that involve a change of ownership to explicitly document > > > that fact. > > > For objects that require additional setup/cleanup, try to make use of > > > open()/close() or request()/release() function name pairings. > > > So gpiod_chip would have open()/close(), gpiod_request_handle would have > > > request()/release(), and the others would all be new()/free()d. > > > > > > > This makes you use a different set of resource management functions > > depending on the object. Additionally some objects can't be created - > > only retrieved from other objects. > > > > Yup, different objects have different lifecycles - even your way. > > > Having a simple ref/unref pair for all opaque data types looks more > > elegant to me. Another real-life example from yesterday: I'm working > > on a small library to go with the new gpio-sim module (meant as a > > replacement for libgpio-mockup) and when I tried to use libmount (to > > verify if configfs is mounted and where) I was super confused by its > > resource management which is similar to what you suggest but also some > > resources need to be freed and some transfer their ownership to other > > objects (without reference counting) and while it's all documented and > > you can find info on that eventually, I still spent time debugging > > strange double-free and other corruptions before I got things right. I > > find libudev and libkmod with their consistent reference counting much > > more instinctive to use. > > > > I find dealing with repeated frees preferable to dealing with issues > related to unexpected side-effects when ownership is unclear. > > > > Conceptually the config for each line can be considered to be independent > > > - the uAPI encoding using attributes and masks is only there to keep the > > > uAPI structs to a manageable size. > > > At this level you can model it as config per line, and only map > > > between the uAPI structures when calling the ioctl()s. > > > So I would drop the gpiod_line_attr, and instead provide accessors and > > > mutators on the gpiod_line_config for each attr type. > > > That removes the whole lifecycle issue for attributes, and allows you to > > > provide a simpler abstraction of the config than that provided in the > > > uAPI. > > > For the mutators there can be two flavours, one that sets the config for > > > the whole set, and another that accepts a subset of applicable lines. > > > Accessors would provide the config attr for a particular line. > > > Both accessor and mutator would use chip offsets to identify the lines. > > > > > > > I had exactly this in mind initially but couldn't find an elegant interface. > > > > There are two options I see: > > > > gpiod_line_config_set_drive(cfg, drive); > > gpiod_line_config_set_drive_index(cfg, drive, index); > > > > or: > > > > gpiod_line_config_set_drive(cfg, drive); > > unsigned int indices[] = { 0, 3, 4, 6 }; > > gpiod_line_config_set_drive_index(cfg, drive, indices); > > > > The former would require a separate function call for each line, the > > former needs a lot of boilerplate code because we'll usually have to > > alloc & fill the indices array. Eventually I decided to go with > > attributes but if you have some ideas/suggestions for that, they're > > welcome! > > > > I generally avoid variadic functions, but this might be an application > where they make sense. i.e. make your indices variadic. > Oh, and make them offsets ;-). > > > > Not sure I like merging the direction and edge into the request_type. > > > I would tend to keep those separate, with set_direction and > > > set_edge_detection functions, with the latter forcing input direction. > > > > > > > If there's no way we can use output with edge detection, why even > > bother with allowing the user to try to set output mode for > > edge_detection or vice-versa? It's just more error checking for no > > benefit, right? The user will still find out that the line is in input > > mode with the line_info. > > > > You are adding another concept for the user to grok - the request_type. > They will need to understand direction and edge detection anyway, why > add a new concept to the mix? > > Oh, and wrt error checking, in my Go library, and as I suggest above, > setting edge detection overrides any previous direction setting and > forces input. Similarly setting output direction cancels any edge > detection. So the config presented to the kernel will always be > consistent and never trigger an EINVAL. And that is all documented in > the corresponding config functions. > > Similarly the drive settings. > > The library policy is last-in-wins, which it can be as there are > distinct calls for each attribute, whereas at the uAPI they are merged > into flags. > > > > I would rename gpiod_request_config to gpiod_request_options. Config > > > is long lived and can be modified, whereas options are one-off. > > > And it would reduce any chance of confusion with gpiod_line_config. > > > > > > > Not sure about this one. With the current naming, it's being made > > clear that we have two separate things to configure, the line(s) and > > the request itself. Naming both "_config" makes a clear distinction. > > Also: the request config can be reused too for separate requests. > > > > I meant one-off wrt a single request. i.e. changing the options after > the lines have been requested can never impact the request, whereas a > changed config can be applied with the set_config(). > > > > gpiod_line_mask should highlight that the bits correspond to lines on > > > the request, in the order provided to gpiod_request_config_set_offsets(), > > > not line offsets on the chip. Perhaps the gpiod_request_config or the > > > gpiod_request_handle should provide the mask functions for a given a > > > qchip offset, rather than require the user to track the mapping? > > > Then the gpiod_line_mask can be opaque as well. > > > > > > > Yes, this is one of those things I struggled with. I wasn't sure which > > one's better because if the user wants to request 64 lines, then set a > > separate config for 20 of them, it's a lot of function calls if we > > allow to set line config by offset of a single line, or we get a > > complicated API with allowing arrays of offsets. > > > > Agreed for the line map functions, it's probably the most confusing > > element of this new API. I'll come up with something. > > > > Ideally I would like to avoid the gpiod_line_mask totally, and always > deal in chip offsets. > > The API should be simplest for the most common use cases. > Those far corner cases can be a pain to use, but must still be possible. > I would put the priority on handling the most common cases: > A. all lines > B. one line > C. a subset of lines > > I would expect A to be the most common by far - that was all that the > previous API supported, then B and then C. > I had suggested providing A and C, with B being a special case of C. > But I could live with C being implemented as multiple calls to B. > > A variadic function could manage all three, with no offsets meaning all > lines? Though in practice I would probably provide distinct functions > for A and B, and reserve the variadic for C. > > Cheers, > Kent. > Thank you for your insight and suggestions. You are right about how the config should be handled and the example with priorities is spot-on. I'm still not sure about the naming for config structures but that's a minor detail. I was on the fence wrt reference counting but then realized that in C++ or Python we still need to provide a mechanism for unconditional closing of chips and releasing of requests. For the former it's because otherwise we'd need to make the object go out of scope manually (probably by storing it in another object that would be "closed" -> pointless abstraction) and in the latter case: Python doesn't even guarantee that the destructor will be called at any specific point. So it seems that it is the right way to go after all and these objects simply will not be shared. In C++ this will be addressed by providing move constructors and operators, in Python reference counting is natural so there's no problem. Same for GLib bindings I want to implement before providing the dbus API. You haven't commented on the line & watch events. I would love to hear your opinion on that too. For v2 I intend to apply the renaming of chip accessors to master and then squash all patches adding the new API into one because splitting them doesn't make the reviewing any easier. Except maybe for the line watch patch which is logically separate. Best regards, Bartosz Golaszewski
On Sat, Apr 17, 2021 at 01:31:01PM +0200, Bartosz Golaszewski wrote: > On Sat, Apr 17, 2021 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Apr 16, 2021 at 11:36:08AM +0200, Bartosz Golaszewski wrote: > > > Hi Kent! > > > > > > Thanks a lot for looking at this. > > > > > > On Wed, Apr 14, 2021 at 4:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote: > > > > > This series introduces an entirely reworked API for the core C part of > > > > > libgpiod. It's not fully functional as the bindings are not modified, > > > > > and starting from patch 5, even the tests stop working. This is on > > > > > purpose as I'd like to reach an agreement on the interface before > > > > > spending time on reworking the tests. > > > > > > > > > > My plan for the development of v2.0 is to keep the changes in a separate > > > > > branch until all bits & pieces are complete and then rearrange them into > > > > > a bisectable series that will be merged into the master branch. > > > > > > > > > > A couple points regarding the design of the new API: > > > > > - all objects have become opaque and can only be accessed using dedicated > > > > > functions > > > > > - line objects as well as bulk containers have been removed > > > > > - line requests are now configured using three types of structures: attributes, > > > > > line config and request config structures, which follows the kernel API > > > > > - line request handles have now a lifetime separate from the parent chips to > > > > > leverage the separation of chip and request file descriptors > > > > > - line events are now opaque but they can be stored in a dedicated container > > > > > so that memory allocations are not necessary everytime an event is read from > > > > > the kernel > > > > > - the library code has been split into several compilation units for easier > > > > > maintenance > > > > > > > > > > The new API is completely documented in include/gpiod.h doxygen comments. > > > > > > > > > > Please let me know what you think. I am aware that these patches are huge and > > > > > difficult to review but I'm really only expecting a general API review - all > > > > > other implementation details can be improved later. > > > > > > > > > > > > > > > Just to clarify why I did certain things the way I did: there are > > > certain semi-standardized guidelines to creating low-level C libraries > > > for linux user-space interfaces. They are gathered in a sample project > > > called libabc[1][2][3]. Most plumbing-layer libraries follow these > > > guidelines more or less rigorously. Many low-level programmers will > > > make certain assumptions based on their previous experiences when > > > working with libraries and it's good to be as little confusing as > > > possible. > > > > > > > Fair enough, though I don't see anything in libabc that conflicts with > > my suggestions, nor that require some of your design choices. > > > > > > In that vein, I'll lump my comments here, rather than scattering them > > > > throughout the patches... > > > > > > > > Overall it feels much tighter and clearer than the old API, though that > > > > could just be personal bias, as it is also closer to the new uAPI. > > > > > > > > I find the ownership and lifetime of objects confusing. I presume you > > > > want to use the reference counting to simplify the object lifecycle, but > > > > that just formalises the possibility that objects are shared - further > > > > confusing the ownership issue. > > > > > > Interesting because in my mind reference counting actually simplifies > > > the life-time of objects - everytime someone (the caller or another > > > object) stores the address of the object, the reference count is > > > increased. > > > > > > Reference counting is a de-facto standard in C libraries. Look at > > > libkmod, libudev, libpulse, all the systemd libraries, the whole of > > > GLib. > > > > > > > I don't doubt that reference counting is out there, I question whether > > it is necessary here. > > > > In my mind reference counting is a tool to be used when you need to > > share objects and it isn't clear who is responsible for freeing > > them. I don't see any need for sharing here at all - for all objects > > (excluding the gpiod_line_attr that I would remove) it is pretty clear > > who is responsible for them at all times. > > > > > Reference counting makes higher-level programming easier. An object > > > lives as long as there's at least one user. In C++ bindings the > > > objects were refcounted using shared_ptr which causes some issues. Now > > > with refcounting implemented at the C library level, we'll be able to > > > use the PImpl pattern with unique_ptr and still keep the objects > > > copyable. > > > > > > > That still sounds to me like you are sharing objects that shouldn't be > > shared. Or copied. > > > > > To summarize: I don't see many disadvantages to using reference > > > counting other than the fact that my documentation is not yet very > > > detailed and lacks some general information about how reference > > > counting works in libgpiod and needs some clarification. > > > > > > > I understand your argument - which is exactly what I had presumed. > > But I still find the idea of having _unref() acting as the universal free > > function to be distasteful. > > > > > > e.g. gpiod_line_config_add_attribute() > > > > - who owns the attr after the call? What happens if it is subsequently > > > > modified? Is the attr copied or does ownership pass to the config? > > > > As written it is neither - the attr becomes shared. How is that > > > > preferable? > > > > > > Yes, it becomes shared. Its reference count is now 2. If the user > > > modifies it, the config will use a modified value at the time of the > > > request. To give you a real-life example of such behavior: in GLib > > > where all GObjects are reference counted, if you pass a reference to a > > > GFile to another user, then set its (GFile's) attribute, they'll be > > > changed for the user sharing the object too. Same with how Python or > > > Java handle references, except you don't need to manage them yourself. > > > > > > > This is all mute if you drop gpiod_line_attr, but I still don't see why > > sharing is preferable in this case. It feels like the worst of both > > worlds - I need to alloc an attr for every add, and I need to unref them > > as well. > > > > > > Similarly gpiod_line_get_consumer() - who owns the returned pointer and > > > > what is it's lifetime? > > > > > > Since simple strings can't be refcounted and this function returns a > > > pointer to a constant string, the life-time of the string is obviously > > > tied to that of the line_info object. The doc should probably say: > > > Returns a pointer to the string stored in the line_info object. > > > > > > > I would dispute the "obviously", particularly since you seem to expect > > everything else to be unref()ed. > > > > No, the ref/unref mechanism is customary for opaque structures. Const > string pointers are always stored in some other object because passing > const char * to free will result in a warning. In libc all functions > that return a malloc()'ed string return char *. > > > > > I would prefer the opaque objects to be able to be free()d normally, and > > > > for any calls that involve a change of ownership to explicitly document > > > > that fact. > > > > For objects that require additional setup/cleanup, try to make use of > > > > open()/close() or request()/release() function name pairings. > > > > So gpiod_chip would have open()/close(), gpiod_request_handle would have > > > > request()/release(), and the others would all be new()/free()d. > > > > > > > > > > This makes you use a different set of resource management functions > > > depending on the object. Additionally some objects can't be created - > > > only retrieved from other objects. > > > > > > > Yup, different objects have different lifecycles - even your way. > > > > > Having a simple ref/unref pair for all opaque data types looks more > > > elegant to me. Another real-life example from yesterday: I'm working > > > on a small library to go with the new gpio-sim module (meant as a > > > replacement for libgpio-mockup) and when I tried to use libmount (to > > > verify if configfs is mounted and where) I was super confused by its > > > resource management which is similar to what you suggest but also some > > > resources need to be freed and some transfer their ownership to other > > > objects (without reference counting) and while it's all documented and > > > you can find info on that eventually, I still spent time debugging > > > strange double-free and other corruptions before I got things right. I > > > find libudev and libkmod with their consistent reference counting much > > > more instinctive to use. > > > > > > > I find dealing with repeated frees preferable to dealing with issues > > related to unexpected side-effects when ownership is unclear. > > > > > > Conceptually the config for each line can be considered to be independent > > > > - the uAPI encoding using attributes and masks is only there to keep the > > > > uAPI structs to a manageable size. > > > > At this level you can model it as config per line, and only map > > > > between the uAPI structures when calling the ioctl()s. > > > > So I would drop the gpiod_line_attr, and instead provide accessors and > > > > mutators on the gpiod_line_config for each attr type. > > > > That removes the whole lifecycle issue for attributes, and allows you to > > > > provide a simpler abstraction of the config than that provided in the > > > > uAPI. > > > > For the mutators there can be two flavours, one that sets the config for > > > > the whole set, and another that accepts a subset of applicable lines. > > > > Accessors would provide the config attr for a particular line. > > > > Both accessor and mutator would use chip offsets to identify the lines. > > > > > > > > > > I had exactly this in mind initially but couldn't find an elegant interface. > > > > > > There are two options I see: > > > > > > gpiod_line_config_set_drive(cfg, drive); > > > gpiod_line_config_set_drive_index(cfg, drive, index); > > > > > > or: > > > > > > gpiod_line_config_set_drive(cfg, drive); > > > unsigned int indices[] = { 0, 3, 4, 6 }; > > > gpiod_line_config_set_drive_index(cfg, drive, indices); > > > > > > The former would require a separate function call for each line, the > > > former needs a lot of boilerplate code because we'll usually have to > > > alloc & fill the indices array. Eventually I decided to go with > > > attributes but if you have some ideas/suggestions for that, they're > > > welcome! > > > > > > > I generally avoid variadic functions, but this might be an application > > where they make sense. i.e. make your indices variadic. > > Oh, and make them offsets ;-). > > > > > > Not sure I like merging the direction and edge into the request_type. > > > > I would tend to keep those separate, with set_direction and > > > > set_edge_detection functions, with the latter forcing input direction. > > > > > > > > > > If there's no way we can use output with edge detection, why even > > > bother with allowing the user to try to set output mode for > > > edge_detection or vice-versa? It's just more error checking for no > > > benefit, right? The user will still find out that the line is in input > > > mode with the line_info. > > > > > > > You are adding another concept for the user to grok - the request_type. > > They will need to understand direction and edge detection anyway, why > > add a new concept to the mix? > > > > Oh, and wrt error checking, in my Go library, and as I suggest above, > > setting edge detection overrides any previous direction setting and > > forces input. Similarly setting output direction cancels any edge > > detection. So the config presented to the kernel will always be > > consistent and never trigger an EINVAL. And that is all documented in > > the corresponding config functions. > > > > Similarly the drive settings. > > > > The library policy is last-in-wins, which it can be as there are > > distinct calls for each attribute, whereas at the uAPI they are merged > > into flags. > > > > > > I would rename gpiod_request_config to gpiod_request_options. Config > > > > is long lived and can be modified, whereas options are one-off. > > > > And it would reduce any chance of confusion with gpiod_line_config. > > > > > > > > > > Not sure about this one. With the current naming, it's being made > > > clear that we have two separate things to configure, the line(s) and > > > the request itself. Naming both "_config" makes a clear distinction. > > > Also: the request config can be reused too for separate requests. > > > > > > > I meant one-off wrt a single request. i.e. changing the options after > > the lines have been requested can never impact the request, whereas a > > changed config can be applied with the set_config(). > > > > > > gpiod_line_mask should highlight that the bits correspond to lines on > > > > the request, in the order provided to gpiod_request_config_set_offsets(), > > > > not line offsets on the chip. Perhaps the gpiod_request_config or the > > > > gpiod_request_handle should provide the mask functions for a given a > > > > qchip offset, rather than require the user to track the mapping? > > > > Then the gpiod_line_mask can be opaque as well. > > > > > > > > > > Yes, this is one of those things I struggled with. I wasn't sure which > > > one's better because if the user wants to request 64 lines, then set a > > > separate config for 20 of them, it's a lot of function calls if we > > > allow to set line config by offset of a single line, or we get a > > > complicated API with allowing arrays of offsets. > > > > > > Agreed for the line map functions, it's probably the most confusing > > > element of this new API. I'll come up with something. > > > > > > > Ideally I would like to avoid the gpiod_line_mask totally, and always > > deal in chip offsets. > > > > The API should be simplest for the most common use cases. > > Those far corner cases can be a pain to use, but must still be possible. > > I would put the priority on handling the most common cases: > > A. all lines > > B. one line > > C. a subset of lines > > > > I would expect A to be the most common by far - that was all that the > > previous API supported, then B and then C. > > I had suggested providing A and C, with B being a special case of C. > > But I could live with C being implemented as multiple calls to B. > > > > A variadic function could manage all three, with no offsets meaning all > > lines? Though in practice I would probably provide distinct functions > > for A and B, and reserve the variadic for C. > > > > Cheers, > > Kent. > > > > Thank you for your insight and suggestions. You are right about how > the config should be handled and the example with priorities is > spot-on. I'm still not sure about the naming for config structures but > that's a minor detail. > I forgot to add that wrt the config mutators, you need to allow overriding of existing config, rather than returning an error on conflict, so that you can change config for the set_config ioctl(). Hence the last-in-wins approach. And as a consequence the mutator is always right and so needs no return code. And you might want to add a copy() for config to allow the user to easily create two slightly different configurations. > I was on the fence wrt reference counting but then realized that in > C++ or Python we still need to provide a mechanism for unconditional > closing of chips and releasing of requests. For the former it's > because otherwise we'd need to make the object go out of scope > manually (probably by storing it in another object that would be > "closed" -> pointless abstraction) and in the latter case: Python > doesn't even guarantee that the destructor will be called at any > specific point. > Hmmm, ok, I was assuming the C++ bindings would wrap the C objects in C++ objects, and the C++ destructor would release any associated resources. For Python I would probably go with an explicit close() or equivalent. My Go library has to use that approach as there are no destructors in Go. > So it seems that it is the right way to go after all and these objects > simply will not be shared. In C++ this will be addressed by providing > move constructors and operators, in Python reference counting is > natural so there's no problem. Same for GLib bindings I want to > implement before providing the dbus API. > > You haven't commented on the line & watch events. I would love to hear > your opinion on that too. > Didn't have any major problems with them on the first pass - nothing worth commenting on anyway. I presume that the info events don't get a buffer as they are expected to be low rate. A zero timeout in the wait() will not block, and just return the event availability? Perhaps the wait() and read() should be strictly orthogonal, i.e. the read() should never block (blocking functions are bad, as per libabc). At least the wait() will eventually timeout (assuming a zero timeout doesn't block forever). Or the two could be combined? Oh, and in the @return for gpiod_line_event_buffer_copy_event(), "decreases" should be "decreased". > For v2 I intend to apply the renaming of chip accessors to master and > then squash all patches adding the new API into one because splitting > them doesn't make the reviewing any easier. Except maybe for the line > watch patch which is logically separate. > Fair enough. In this case I applied all the patches and looked at the resulting gpiod.h, as I wasn't terribly interested in the changes - only the final result. Cheers, Kent. > Best regards, > Bartosz Golaszewski
On Sun, Apr 18, 2021 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sat, Apr 17, 2021 at 01:31:01PM +0200, Bartosz Golaszewski wrote: > > On Sat, Apr 17, 2021 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Fri, Apr 16, 2021 at 11:36:08AM +0200, Bartosz Golaszewski wrote: > > > > Hi Kent! > > > > > > > > Thanks a lot for looking at this. > > > > > > > > On Wed, Apr 14, 2021 at 4:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote: > > > > > > This series introduces an entirely reworked API for the core C part of > > > > > > libgpiod. It's not fully functional as the bindings are not modified, > > > > > > and starting from patch 5, even the tests stop working. This is on > > > > > > purpose as I'd like to reach an agreement on the interface before > > > > > > spending time on reworking the tests. > > > > > > > > > > > > My plan for the development of v2.0 is to keep the changes in a separate > > > > > > branch until all bits & pieces are complete and then rearrange them into > > > > > > a bisectable series that will be merged into the master branch. > > > > > > > > > > > > A couple points regarding the design of the new API: > > > > > > - all objects have become opaque and can only be accessed using dedicated > > > > > > functions > > > > > > - line objects as well as bulk containers have been removed > > > > > > - line requests are now configured using three types of structures: attributes, > > > > > > line config and request config structures, which follows the kernel API > > > > > > - line request handles have now a lifetime separate from the parent chips to > > > > > > leverage the separation of chip and request file descriptors > > > > > > - line events are now opaque but they can be stored in a dedicated container > > > > > > so that memory allocations are not necessary everytime an event is read from > > > > > > the kernel > > > > > > - the library code has been split into several compilation units for easier > > > > > > maintenance > > > > > > > > > > > > The new API is completely documented in include/gpiod.h doxygen comments. > > > > > > > > > > > > Please let me know what you think. I am aware that these patches are huge and > > > > > > difficult to review but I'm really only expecting a general API review - all > > > > > > other implementation details can be improved later. > > > > > > > > > > > > > > > > > > > Just to clarify why I did certain things the way I did: there are > > > > certain semi-standardized guidelines to creating low-level C libraries > > > > for linux user-space interfaces. They are gathered in a sample project > > > > called libabc[1][2][3]. Most plumbing-layer libraries follow these > > > > guidelines more or less rigorously. Many low-level programmers will > > > > make certain assumptions based on their previous experiences when > > > > working with libraries and it's good to be as little confusing as > > > > possible. > > > > > > > > > > Fair enough, though I don't see anything in libabc that conflicts with > > > my suggestions, nor that require some of your design choices. > > > > > > > > In that vein, I'll lump my comments here, rather than scattering them > > > > > throughout the patches... > > > > > > > > > > Overall it feels much tighter and clearer than the old API, though that > > > > > could just be personal bias, as it is also closer to the new uAPI. > > > > > > > > > > I find the ownership and lifetime of objects confusing. I presume you > > > > > want to use the reference counting to simplify the object lifecycle, but > > > > > that just formalises the possibility that objects are shared - further > > > > > confusing the ownership issue. > > > > > > > > Interesting because in my mind reference counting actually simplifies > > > > the life-time of objects - everytime someone (the caller or another > > > > object) stores the address of the object, the reference count is > > > > increased. > > > > > > > > Reference counting is a de-facto standard in C libraries. Look at > > > > libkmod, libudev, libpulse, all the systemd libraries, the whole of > > > > GLib. > > > > > > > > > > I don't doubt that reference counting is out there, I question whether > > > it is necessary here. > > > > > > In my mind reference counting is a tool to be used when you need to > > > share objects and it isn't clear who is responsible for freeing > > > them. I don't see any need for sharing here at all - for all objects > > > (excluding the gpiod_line_attr that I would remove) it is pretty clear > > > who is responsible for them at all times. > > > > > > > Reference counting makes higher-level programming easier. An object > > > > lives as long as there's at least one user. In C++ bindings the > > > > objects were refcounted using shared_ptr which causes some issues. Now > > > > with refcounting implemented at the C library level, we'll be able to > > > > use the PImpl pattern with unique_ptr and still keep the objects > > > > copyable. > > > > > > > > > > That still sounds to me like you are sharing objects that shouldn't be > > > shared. Or copied. > > > > > > > To summarize: I don't see many disadvantages to using reference > > > > counting other than the fact that my documentation is not yet very > > > > detailed and lacks some general information about how reference > > > > counting works in libgpiod and needs some clarification. > > > > > > > > > > I understand your argument - which is exactly what I had presumed. > > > But I still find the idea of having _unref() acting as the universal free > > > function to be distasteful. > > > > > > > > e.g. gpiod_line_config_add_attribute() > > > > > - who owns the attr after the call? What happens if it is subsequently > > > > > modified? Is the attr copied or does ownership pass to the config? > > > > > As written it is neither - the attr becomes shared. How is that > > > > > preferable? > > > > > > > > Yes, it becomes shared. Its reference count is now 2. If the user > > > > modifies it, the config will use a modified value at the time of the > > > > request. To give you a real-life example of such behavior: in GLib > > > > where all GObjects are reference counted, if you pass a reference to a > > > > GFile to another user, then set its (GFile's) attribute, they'll be > > > > changed for the user sharing the object too. Same with how Python or > > > > Java handle references, except you don't need to manage them yourself. > > > > > > > > > > This is all mute if you drop gpiod_line_attr, but I still don't see why > > > sharing is preferable in this case. It feels like the worst of both > > > worlds - I need to alloc an attr for every add, and I need to unref them > > > as well. > > > > > > > > Similarly gpiod_line_get_consumer() - who owns the returned pointer and > > > > > what is it's lifetime? > > > > > > > > Since simple strings can't be refcounted and this function returns a > > > > pointer to a constant string, the life-time of the string is obviously > > > > tied to that of the line_info object. The doc should probably say: > > > > Returns a pointer to the string stored in the line_info object. > > > > > > > > > > I would dispute the "obviously", particularly since you seem to expect > > > everything else to be unref()ed. > > > > > > > No, the ref/unref mechanism is customary for opaque structures. Const > > string pointers are always stored in some other object because passing > > const char * to free will result in a warning. In libc all functions > > that return a malloc()'ed string return char *. > > > > > > > I would prefer the opaque objects to be able to be free()d normally, and > > > > > for any calls that involve a change of ownership to explicitly document > > > > > that fact. > > > > > For objects that require additional setup/cleanup, try to make use of > > > > > open()/close() or request()/release() function name pairings. > > > > > So gpiod_chip would have open()/close(), gpiod_request_handle would have > > > > > request()/release(), and the others would all be new()/free()d. > > > > > > > > > > > > > This makes you use a different set of resource management functions > > > > depending on the object. Additionally some objects can't be created - > > > > only retrieved from other objects. > > > > > > > > > > Yup, different objects have different lifecycles - even your way. > > > > > > > Having a simple ref/unref pair for all opaque data types looks more > > > > elegant to me. Another real-life example from yesterday: I'm working > > > > on a small library to go with the new gpio-sim module (meant as a > > > > replacement for libgpio-mockup) and when I tried to use libmount (to > > > > verify if configfs is mounted and where) I was super confused by its > > > > resource management which is similar to what you suggest but also some > > > > resources need to be freed and some transfer their ownership to other > > > > objects (without reference counting) and while it's all documented and > > > > you can find info on that eventually, I still spent time debugging > > > > strange double-free and other corruptions before I got things right. I > > > > find libudev and libkmod with their consistent reference counting much > > > > more instinctive to use. > > > > > > > > > > I find dealing with repeated frees preferable to dealing with issues > > > related to unexpected side-effects when ownership is unclear. > > > > > > > > Conceptually the config for each line can be considered to be independent > > > > > - the uAPI encoding using attributes and masks is only there to keep the > > > > > uAPI structs to a manageable size. > > > > > At this level you can model it as config per line, and only map > > > > > between the uAPI structures when calling the ioctl()s. > > > > > So I would drop the gpiod_line_attr, and instead provide accessors and > > > > > mutators on the gpiod_line_config for each attr type. > > > > > That removes the whole lifecycle issue for attributes, and allows you to > > > > > provide a simpler abstraction of the config than that provided in the > > > > > uAPI. > > > > > For the mutators there can be two flavours, one that sets the config for > > > > > the whole set, and another that accepts a subset of applicable lines. > > > > > Accessors would provide the config attr for a particular line. > > > > > Both accessor and mutator would use chip offsets to identify the lines. > > > > > > > > > > > > > I had exactly this in mind initially but couldn't find an elegant interface. > > > > > > > > There are two options I see: > > > > > > > > gpiod_line_config_set_drive(cfg, drive); > > > > gpiod_line_config_set_drive_index(cfg, drive, index); > > > > > > > > or: > > > > > > > > gpiod_line_config_set_drive(cfg, drive); > > > > unsigned int indices[] = { 0, 3, 4, 6 }; > > > > gpiod_line_config_set_drive_index(cfg, drive, indices); > > > > > > > > The former would require a separate function call for each line, the > > > > former needs a lot of boilerplate code because we'll usually have to > > > > alloc & fill the indices array. Eventually I decided to go with > > > > attributes but if you have some ideas/suggestions for that, they're > > > > welcome! > > > > > > > > > > I generally avoid variadic functions, but this might be an application > > > where they make sense. i.e. make your indices variadic. > > > Oh, and make them offsets ;-). > > > > > > > > Not sure I like merging the direction and edge into the request_type. > > > > > I would tend to keep those separate, with set_direction and > > > > > set_edge_detection functions, with the latter forcing input direction. > > > > > > > > > > > > > If there's no way we can use output with edge detection, why even > > > > bother with allowing the user to try to set output mode for > > > > edge_detection or vice-versa? It's just more error checking for no > > > > benefit, right? The user will still find out that the line is in input > > > > mode with the line_info. > > > > > > > > > > You are adding another concept for the user to grok - the request_type. > > > They will need to understand direction and edge detection anyway, why > > > add a new concept to the mix? > > > > > > Oh, and wrt error checking, in my Go library, and as I suggest above, > > > setting edge detection overrides any previous direction setting and > > > forces input. Similarly setting output direction cancels any edge > > > detection. So the config presented to the kernel will always be > > > consistent and never trigger an EINVAL. And that is all documented in > > > the corresponding config functions. > > > > > > Similarly the drive settings. > > > > > > The library policy is last-in-wins, which it can be as there are > > > distinct calls for each attribute, whereas at the uAPI they are merged > > > into flags. > > > > > > > > I would rename gpiod_request_config to gpiod_request_options. Config > > > > > is long lived and can be modified, whereas options are one-off. > > > > > And it would reduce any chance of confusion with gpiod_line_config. > > > > > > > > > > > > > Not sure about this one. With the current naming, it's being made > > > > clear that we have two separate things to configure, the line(s) and > > > > the request itself. Naming both "_config" makes a clear distinction. > > > > Also: the request config can be reused too for separate requests. > > > > > > > > > > I meant one-off wrt a single request. i.e. changing the options after > > > the lines have been requested can never impact the request, whereas a > > > changed config can be applied with the set_config(). > > > > > > > > gpiod_line_mask should highlight that the bits correspond to lines on > > > > > the request, in the order provided to gpiod_request_config_set_offsets(), > > > > > not line offsets on the chip. Perhaps the gpiod_request_config or the > > > > > gpiod_request_handle should provide the mask functions for a given a > > > > > qchip offset, rather than require the user to track the mapping? > > > > > Then the gpiod_line_mask can be opaque as well. > > > > > > > > > > > > > Yes, this is one of those things I struggled with. I wasn't sure which > > > > one's better because if the user wants to request 64 lines, then set a > > > > separate config for 20 of them, it's a lot of function calls if we > > > > allow to set line config by offset of a single line, or we get a > > > > complicated API with allowing arrays of offsets. > > > > > > > > Agreed for the line map functions, it's probably the most confusing > > > > element of this new API. I'll come up with something. > > > > > > > > > > Ideally I would like to avoid the gpiod_line_mask totally, and always > > > deal in chip offsets. > > > > > > The API should be simplest for the most common use cases. > > > Those far corner cases can be a pain to use, but must still be possible. > > > I would put the priority on handling the most common cases: > > > A. all lines > > > B. one line > > > C. a subset of lines > > > > > > I would expect A to be the most common by far - that was all that the > > > previous API supported, then B and then C. > > > I had suggested providing A and C, with B being a special case of C. > > > But I could live with C being implemented as multiple calls to B. > > > > > > A variadic function could manage all three, with no offsets meaning all > > > lines? Though in practice I would probably provide distinct functions > > > for A and B, and reserve the variadic for C. > > > > > > Cheers, > > > Kent. > > > > > > > Thank you for your insight and suggestions. You are right about how > > the config should be handled and the example with priorities is > > spot-on. I'm still not sure about the naming for config structures but > > that's a minor detail. > > > > I forgot to add that wrt the config mutators, you need to allow > overriding of existing config, rather than returning an error on > conflict, so that you can change config for the set_config ioctl(). > Hence the last-in-wins approach. And as a consequence the mutator is > always right and so needs no return code. > This sounds good in theory but how do we handle a situation that requires more than 10 attributes? Override the first one? The last one? What if the line offsets passed to the request config repeat themselves? I think some sanitization of input is in order. Regarding offsets: I was thinking about how to approach referring to lines in configs and requests by offsets only (in order to hide the whole masking logic) and while for a request (for example: when setting/reading line) this is straightforward (as long as we make sure the offsets are never duplicated), the line config structure doesn't really know the concept of offsets. So when we set a config option for a specific line, we need to carry the offset information somehow in the structure until the request is actually made. How do you deal with this in your library? Did you expose any of the bitmap details in your API? Can we really avoid dealing with indexing of lines in a request? > And you might want to add a copy() for config to allow the user to > easily create two slightly different configurations. > > > I was on the fence wrt reference counting but then realized that in > > C++ or Python we still need to provide a mechanism for unconditional > > closing of chips and releasing of requests. For the former it's > > because otherwise we'd need to make the object go out of scope > > manually (probably by storing it in another object that would be > > "closed" -> pointless abstraction) and in the latter case: Python > > doesn't even guarantee that the destructor will be called at any > > specific point. > > > > Hmmm, ok, I was assuming the C++ bindings would wrap the C objects in C++ > objects, and the C++ destructor would release any associated resources. > Yes, but what if the user wants to close the chip or release the request without the underlying object going out of scope? I think we need to keep that possibility. > For Python I would probably go with an explicit close() or equivalent. > My Go library has to use that approach as there are no destructors in Go. > > > So it seems that it is the right way to go after all and these objects > > simply will not be shared. In C++ this will be addressed by providing > > move constructors and operators, in Python reference counting is > > natural so there's no problem. Same for GLib bindings I want to > > implement before providing the dbus API. > > > > You haven't commented on the line & watch events. I would love to hear > > your opinion on that too. > > > > Didn't have any major problems with them on the first pass - nothing > worth commenting on anyway. > > I presume that the info events don't get a buffer as they are > expected to be low rate. > Indeed. > A zero timeout in the wait() will not block, and just return the event > availability? > Yes. > Perhaps the wait() and read() should be strictly orthogonal, i.e. the > read() should never block (blocking functions are bad, as per libabc). > At least the wait() will eventually timeout (assuming a zero timeout > doesn't block forever). > Or the two could be combined? > Good point, but I'd prefer to keep that logic separate. I've never seen a combined poll()/read() in any lib. I will see about that non-blocking read() though. Bart > Oh, and in the @return for gpiod_line_event_buffer_copy_event(), > "decreases" should be "decreased". > > > For v2 I intend to apply the renaming of chip accessors to master and > > then squash all patches adding the new API into one because splitting > > them doesn't make the reviewing any easier. Except maybe for the line > > watch patch which is logically separate. > > > > Fair enough. In this case I applied all the patches and looked at the > resulting gpiod.h, as I wasn't terribly interested in the changes - only > the final result. > > Cheers, > Kent. > > > Best regards, > > Bartosz Golaszewski
On Sun, Apr 18, 2021 at 11:12:24PM +0200, Bartosz Golaszewski wrote: > On Sun, Apr 18, 2021 at 5:48 AM Kent Gibson <warthog618@gmail.com> wrote: > > [snip!] > > > > I forgot to add that wrt the config mutators, you need to allow > > overriding of existing config, rather than returning an error on > > conflict, so that you can change config for the set_config ioctl(). > > Hence the last-in-wins approach. And as a consequence the mutator is > > always right and so needs no return code. > > > > This sounds good in theory but how do we handle a situation that > requires more than 10 attributes? Override the first one? The last > one? What if the line offsets passed to the request config repeat > themselves? I think some sanitization of input is in order. > Repeating of lines is equivalent to repeatedly setting a bit, so the subsequent instances are ignored. In practice I don't even need to check - if the user includes the line multiple times then it gets set multiple times - to the same thing. The case where a complex config can't be mapped to the uAPI, e.g. due to too many attributes on too many lines, is handled at the time of the request_lines() or set_config() itself when that mapping is performed. Those will return an "overly complex config" error. > Regarding offsets: I was thinking about how to approach referring to > lines in configs and requests by offsets only (in order to hide the > whole masking logic) and while for a request (for example: when > setting/reading line) this is straightforward (as long as we make sure > the offsets are never duplicated), the line config structure doesn't > really know the concept of offsets. So when we set a config option for > a specific line, we need to carry the offset information somehow in > the structure until the request is actually made. How do you deal with > this in your library? Did you expose any of the bitmap details in your > API? Can we really avoid dealing with indexing of lines in a request? > In the request config I use a map of offset to line config to avoid duplication. A config change that alters any existing setting just overwrites the old. The line config is similar to your struct gpiod_line_config. The line config for a particular line is only created and added to the map if there is a config change specific to that line. Each attribute has a "not set" value, in which case the request-wide default is used. The request-wide default config is stored separately from the map. And there is a function to reset a line config back to the default, i.e. drop that line config from the map. The request_lines() and set_config(), that accept the config, also have the list of offsets available (provided to the request_lines() and subsequently stored as part of the request struct for the set_config()) and so can map from offsets to indices to build the bitmap. The bitmap and indices themselves are never exposed. That is a high level description - the details are actually a little different as the Go implementation uses functional options, so the initial config settings become parameters to the request, and bundles the config into the request object itself. > > And you might want to add a copy() for config to allow the user to > > easily create two slightly different configurations. > > > > > I was on the fence wrt reference counting but then realized that in > > > C++ or Python we still need to provide a mechanism for unconditional > > > closing of chips and releasing of requests. For the former it's > > > because otherwise we'd need to make the object go out of scope > > > manually (probably by storing it in another object that would be > > > "closed" -> pointless abstraction) and in the latter case: Python > > > doesn't even guarantee that the destructor will be called at any > > > specific point. > > > > > > > Hmmm, ok, I was assuming the C++ bindings would wrap the C objects in C++ > > objects, and the C++ destructor would release any associated resources. > > > > Yes, but what if the user wants to close the chip or release the > request without the underlying object going out of scope? I think we > need to keep that possibility. > Then you also provide a close() method. They aren't mutually exclusive. Cheers, Kent.
On Mon, Apr 19, 2021 at 3:17 AM Kent Gibson <warthog618@gmail.com> wrote:
>
[snip -> long discussion on the libgpiod C API]
Hi Kent,
I was working on the next iteration of the code and I'm struggling
with the implementation of some elements without the concept of
attributes.
So I initially liked the idea of variadic functions but they won't
work for language bindings so that's a no go. On that note: I wanted
to get some inspiration from your go library but your elegant API
makes use of go features (like interfaces) we don't have in C.
The problem we have is basically about implementing primary and
secondary line configuration objects - where the primary contains the
default config for all requested lines and the secondary can override
certain config options (should zeroed values for enumerated types mean
- don't override?) for certain lines.
The basic line config structure (let's call it struct
gpiod_line_config) can be very simple and have the following mutators:
struct gpiod_line_config *cfg = gpiod_line_config_new();
gpiod_line_config_set_direction(cfg, GPIOD_LINE_CONFIG_DIRECTION_OUTPUT);
gpiod_line_config_set_active_low(cfg, true);
and so on for for drive, bias, edge, debounce, realtime clock.
Output values would be set like this:
unsigned int values[] = { 0, 1, 1, 0 }, num_lines = 4;
gpiod_line_config_set_output_values(cfg, num_lines, values);
One can imagine a simple request with the same config for all lines as:
gpiod_chip_request_lines(chip, req_cfg, line_cfg);
Where req_cfg configures request-specific options, and line_cfg
contains the above line config. I'm still not convinced that
gpiod_request_options is the better name, I think I prefer the
juxtaposition of the two names: line_config and request_config.
Now how do we pass a composite line config with overridden values in C
without interfaces etc.?
One idea I have is to add a new object called struct
gpiod_line_config_ext (for extended) that would take one primary
config and an arbitrary number of secondary configs with the following
example use-case:
struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new();
unsigned int offsets[] = { 2, 3 };
/* Add the default config for this request. */
gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg);
/* Add a secondary config for 2 lines with offsets: 2 and 3. */
gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets);
gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg);
Does this make sense? I'm worried about the resource management here.
Who should be responsible for freeing the config structures? Should
the extended config take ownership? Should the user remain
responsible? Back to reference counting for these objects? Is this
even a good idea?
Please let me know what you think, I could use some advice.
Best Regards,
Bartosz
On Wed, Apr 21, 2021 at 10:04:04PM +0200, Bartosz Golaszewski wrote: > On Mon, Apr 19, 2021 at 3:17 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip -> long discussion on the libgpiod C API] > > Hi Kent, > > I was working on the next iteration of the code and I'm struggling > with the implementation of some elements without the concept of > attributes. > > So I initially liked the idea of variadic functions but they won't > work for language bindings so that's a no go. On that note: I wanted > to get some inspiration from your go library but your elegant API > makes use of go features (like interfaces) we don't have in C. > It is the functional options that is the big difference between my Go implementation and what I would do in C. I happen to use interfaces to implement those options. You could do something similar in C (cos what can't you do in C) but it would be weird, so lets not go there. You can still provide the variadic forms for C users. And couldn't the language bindings use the "v" variant, as libabc suggests?: "Be careful with variadic functions - It's great if you provide them, but you must accompany them with "v" variants (i.e. functions taking a va_arg object), and provide non-variadic variants as well. This is important to get language wrappers right." > The problem we have is basically about implementing primary and > secondary line configuration objects - where the primary contains the > default config for all requested lines and the secondary can override > certain config options (should zeroed values for enumerated types mean > - don't override?) for certain lines. > Yep, use the 0 value to mean "defaulted". For the secondary that means use the primary value. For the primary that means use the kernel default, so the primary is initialised with the kernel defaults. Note that accessors, if provided, generally wouldn't return the 0 value to the user - they follow the secondary -> primary chain and return the effective setting. > The basic line config structure (let's call it struct > gpiod_line_config) can be very simple and have the following mutators: > This is where you are immediately off into the weeds, so I obviously didn't communicate my suggestion very well... . The opaque config object presented to the user is not the simple line config object - it is the container for the whole request config (which is different from the request options - which is exactly why I would like the options not to be called request_config). The user never sees the simple line config, which is internal, only the request config. The accessors always work on the request config at attribute level, and there is never a need to apply or return the whole config for a particular line. You could - but it is not necessary for core functionality, so for now don't. I realise this makes the internal modelling of config much more complicated, but the goal is to provide a simplified interface for the user - so it should be expected that the majority of the complexity will end up in the library rather than user code. In my Go implementation I merge the request config into the request object itself. You could also do that, though the semantics might be clearer if you keep them separate (more on that later). > struct gpiod_line_config *cfg = gpiod_line_config_new(); > > gpiod_line_config_set_active_low(cfg, true); I would provide two functions for active level - set_active_high() and set_active_low(). Even if you don't, the function here should be set_active_level(), in case you want to add them later. > gpiod_line_config_set_direction(cfg, GPIOD_LINE_CONFIG_DIRECTION_OUTPUT); > That is the A case, i.e. the whole request. How would you name the B and C cases? _set_<attr>_line and _set_<attr>_lines? or _set_line_<attr> and _set_lines_<attr>? or something else? It might be worthwhile providing separate set_input() and set_output() functions - as the output case requires the initial value. You could have the user call set_output_values(), but why force them to make another call? > and so on for for drive, bias, edge, debounce, realtime clock. > > Output values would be set like this: > > unsigned int values[] = { 0, 1, 1, 0 }, num_lines = 4; > gpiod_line_config_set_output_values(cfg, num_lines, values); > I don't like the implicit identification of lines based on request ordering here. I realise my Go API does that, but that is an option to the request itself, so the requested lines are also present and visible to the casual reader, whereas this is a standalone call. So it should require the list of lines that you are setting the values for. i.e. it is the mutator for a subset of lines, so the C case. And it implicitly sets those lines to outputs, so it can be more clearly named set_output(value) (that is the A case, btw). > One can imagine a simple request with the same config for all lines as: > > gpiod_chip_request_lines(chip, req_cfg, line_cfg); > > Where req_cfg configures request-specific options, and line_cfg > contains the above line config. I'm still not convinced that > gpiod_request_options is the better name, I think I prefer the > juxtaposition of the two names: line_config and request_config. > That's ok - I'm pretty sure you'll get there eventually ;-). > Now how do we pass a composite line config with overridden values in C > without interfaces etc.? > As above, the req_cfg is the composite line config, so req = gpiod_chip_request_lines(chip, req_options, req_cfg); Or if you were to merge the request config, and even the options, into the request: unsigned int lines[] = { 0, 4, 12, 54 }, num_lines = 4; req = gpiod_line_request_new(num_lines, lines); // also variadic forms?? // call req option and config mutators here... gpiod_line_request_set_active_low(req); gpiod_line_request_set_output(req, 1); gpiod_line_request_set_line_input(req, 12); gpiod_line_request_set_event_buffer_size(req, 42); ... // then actually request the lines... err = gpiod_chip_request_lines(chip, req); which may error for various reasons, such as lines already being requested or overly complex config. Merging everything into the request means fewer opaque objects and interactions for the user to have to deal with, which is always a good thing. The downside is that changes to options and config, such as the gpiod_line_request_set_active_low() etc here, are not applied until either the gpiod_chip_request_lines() or the set_config() call, which could be confusing. Though the more I think about it the more I think the resulting simplification of the API wins out. i.e. these objects: struct gpiod_line_attr; struct gpiod_line_config; struct gpiod_request_config; struct gpiod_request_handle; all get collapsed into: struct gpiod_line_request; which significantly reduces the cognitive load on the user. The set_config() would probably be called something like: err = gpiod_line_request_reconfigure(req) to distinguish it from the mutators which use the _set_ naming. (and it would also align with my Go library ;-) > One idea I have is to add a new object called struct > gpiod_line_config_ext (for extended) that would take one primary > config and an arbitrary number of secondary configs with the following > example use-case: > > struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new(); > unsigned int offsets[] = { 2, 3 }; > > /* Add the default config for this request. */ > gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg); > /* Add a secondary config for 2 lines with offsets: 2 and 3. */ > gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets); > > gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg); > Please, no _ext objects - that is an admission of failure right there. > Does this make sense? I'm worried about the resource management here. > Who should be responsible for freeing the config structures? Should > the extended config take ownership? Should the user remain > responsible? Back to reference counting for these objects? Is this > even a good idea? > The user is responsible for freeing the request config. The request config is responsible for freeing the line configs, if there are any - the user isn't even aware of them so they obviously can't. Similarly if you merge the config into the request. > Please let me know what you think, I could use some advice. Hopefully I've communicated my meaning a little more clearly this time? Cheers, Kent.
On Thu, Apr 22, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Apr 21, 2021 at 10:04:04PM +0200, Bartosz Golaszewski wrote: > > On Mon, Apr 19, 2021 at 3:17 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > [snip -> long discussion on the libgpiod C API] > > > > Hi Kent, > > > > I was working on the next iteration of the code and I'm struggling > > with the implementation of some elements without the concept of > > attributes. > > > > So I initially liked the idea of variadic functions but they won't > > work for language bindings so that's a no go. On that note: I wanted > > to get some inspiration from your go library but your elegant API > > makes use of go features (like interfaces) we don't have in C. > > > > It is the functional options that is the big difference between my Go > implementation and what I would do in C. I happen to use interfaces to > implement those options. You could do something similar in C (cos what > can't you do in C) but it would be weird, so lets not go there. > > You can still provide the variadic forms for C users. > And couldn't the language bindings use the "v" variant, as libabc > suggests?: > > "Be careful with variadic functions > - It's great if you provide them, but you must accompany them with > "v" variants (i.e. functions taking a va_arg object), and provide > non-variadic variants as well. This is important to get language > wrappers right." > The "v" functions do nothing for language bindings - you can't construct the required va_list out of thin air. What you do usually is this (example taken from GLib): gpointer g_object_new(GType object_type, const gchar *first_property_name, ...); has a non-variadic counterpart: GObject * g_object_new_with_properties(GType object_type, guint n_properties, const char *names[], const GValue values[]); I would prefer to simply not do variadic functions. The low-level API should in general be wrapped in high-level languages. Most people that mail me on libgpiod seem to use Python anyway. > > The problem we have is basically about implementing primary and > > secondary line configuration objects - where the primary contains the > > default config for all requested lines and the secondary can override > > certain config options (should zeroed values for enumerated types mean > > - don't override?) for certain lines. > > > > Yep, use the 0 value to mean "defaulted". > For the secondary that means use the primary value. > For the primary that means use the kernel default, so the primary is > initialised with the kernel defaults. > > Note that accessors, if provided, generally wouldn't return the 0 value to > the user - they follow the secondary -> primary chain and return the > effective setting. > > > The basic line config structure (let's call it struct > > gpiod_line_config) can be very simple and have the following mutators: > > > > This is where you are immediately off into the weeds, so I obviously > didn't communicate my suggestion very well... > . > The opaque config object presented to the user is not the simple line > config object - it is the container for the whole request config (which > is different from the request options - which is exactly why I would like > the options not to be called request_config). > > The user never sees the simple line config, which is internal, only the > request config. > The accessors always work on the request config at attribute level, and > there is never a need to apply or return the whole config for a particular > line. You could - but it is not necessary for core functionality, so for > now don't. > > I realise this makes the internal modelling of config much more > complicated, but the goal is to provide a simplified interface for the > user - so it should be expected that the majority of the complexity will > end up in the library rather than user code. > Yeah, I understood this alright but I figured that this will bloat the library interface with three variants of mutators per every config option (or more if we provide two functions for direction and active level) more than having simpler config objects and applying them globally or per line. > In my Go implementation I merge the request config into the request > object itself. You could also do that, though the semantics might be > clearer if you keep them separate (more on that later). > My goal was to logically split the config into elements that can be reconfigured and those that cannot. More on that later. > > struct gpiod_line_config *cfg = gpiod_line_config_new(); > > > > gpiod_line_config_set_active_low(cfg, true); > > I would provide two functions for active level - set_active_high() and > set_active_low(). Even if you don't, the function here should be > set_active_level(), in case you want to add them later. > > > gpiod_line_config_set_direction(cfg, GPIOD_LINE_CONFIG_DIRECTION_OUTPUT); > > > > That is the A case, i.e. the whole request. How would you name the B > and C cases? > > _set_<attr>_line and _set_<attr>_lines? > > or _set_line_<attr> and _set_lines_<attr>? > > or something else? > These are what I had in mind and also: _offset and _offsets. > It might be worthwhile providing separate set_input() and > set_output() functions - as the output case requires the initial value. > You could have the user call set_output_values(), but why force them to > make another call? > > > and so on for for drive, bias, edge, debounce, realtime clock. > > > > Output values would be set like this: > > > > unsigned int values[] = { 0, 1, 1, 0 }, num_lines = 4; > > gpiod_line_config_set_output_values(cfg, num_lines, values); > > > > I don't like the implicit identification of lines based on request > ordering here. I realise my Go API does that, but that is an option to > the request itself, so the requested lines are also present and visible > to the casual reader, whereas this is a standalone call. > > So it should require the list of lines that you are setting the values > for. i.e. it is the mutator for a subset of lines, so the C case. > > And it implicitly sets those lines to outputs, so it can be more clearly > named set_output(value) (that is the A case, btw). > I can imagine the B case like: gpiod_line_config_set_output_value(config, offset, value); But how would exactly the call for the A case look like? Two arrays with offset -> value mapping? unsigned int offsets[] = { 0, 2, 5 }, values[] = { 0, 1 ,1 }; gpiod_line_config_set_output_values_offsets(config, 3, offsets, values); ? > > One can imagine a simple request with the same config for all lines as: > > > > gpiod_chip_request_lines(chip, req_cfg, line_cfg); > > > > Where req_cfg configures request-specific options, and line_cfg > > contains the above line config. I'm still not convinced that > > gpiod_request_options is the better name, I think I prefer the > > juxtaposition of the two names: line_config and request_config. > > > > That's ok - I'm pretty sure you'll get there eventually ;-). > > > Now how do we pass a composite line config with overridden values in C > > without interfaces etc.? > > > > As above, the req_cfg is the composite line config, so > > req = gpiod_chip_request_lines(chip, req_options, req_cfg); > > Or if you were to merge the request config, and even the options, into the > request: > > unsigned int lines[] = { 0, 4, 12, 54 }, num_lines = 4; > req = gpiod_line_request_new(num_lines, lines); // also variadic forms?? > // call req option and config mutators here... > gpiod_line_request_set_active_low(req); > gpiod_line_request_set_output(req, 1); > gpiod_line_request_set_line_input(req, 12); > gpiod_line_request_set_event_buffer_size(req, 42); > ... > // then actually request the lines... > err = gpiod_chip_request_lines(chip, req); > > which may error for various reasons, such as lines already being > requested or overly complex config. > > Merging everything into the request means fewer opaque objects and > interactions for the user to have to deal with, which is always a good > thing. > The downside is that changes to options and config, such as the > gpiod_line_request_set_active_low() etc here, are not applied until > either the gpiod_chip_request_lines() or the set_config() call, which > could be confusing. Though the more I think about it the more I think > the resulting simplification of the API wins out. i.e. these objects: > > struct gpiod_line_attr; > struct gpiod_line_config; > struct gpiod_request_config; > struct gpiod_request_handle; > > all get collapsed into: > > struct gpiod_line_request; > > which significantly reduces the cognitive load on the user. > > The set_config() would probably be called something like: > > err = gpiod_line_request_reconfigure(req) > This lack of splitting of options into configurable and constant ones visually suggests that you can change all request options later on which is not true. I think that at least for the C API, we should split the responsibilities of objects and keep the division into request config, line config *and* the line handle whose lifetime is from the moment the lines get requested until they're released. > to distinguish it from the mutators which use the _set_ naming. > (and it would also align with my Go library ;-) > > > One idea I have is to add a new object called struct > > gpiod_line_config_ext (for extended) that would take one primary > > config and an arbitrary number of secondary configs with the following > > example use-case: > > > > struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new(); > > unsigned int offsets[] = { 2, 3 }; > > > > /* Add the default config for this request. */ > > gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg); > > /* Add a secondary config for 2 lines with offsets: 2 and 3. */ > > gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets); > > > > gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg); > > > > Please, no _ext objects - that is an admission of failure right there. > I wanted to protest but then realized that if you need _ext interfaces then it means your non-extended, initial design is already flawed. :) Ok so let's try again. How about: Three structs: struct gpiod_line_config; struct gpiod_request_config; struct gpiod_line_request; The first one holds the composite of primary and secondary configs and is modified using mutators according to this scheme: gpiod_line_config_set_<attr>(config, attr); gpiod_line_config_set_<attr>_offset(config, attr, offset); gpiod_line_config_set_<attr>_offsets(config, attr, num_offsets, offsets); With notable exceptions for: gpiod_line_config_set_[input|active_low|active_high](config); gpiod_line_config_set_[input|active_low|active_high]_offset(config, offset); gpiod_line_config_set_[input|active_low|active_high]_offsets(config, num_offsets, offsets); and: gpiod_line_config_set_output(config, num_lines, offsets, values); gpiod_line_config_set_output_offset(config, offset, value); The request function takes a single line config and a request config and returns a new gpiod_line_request like in the first iteration. Then the lines can be set like this: // num_lines refers to the number of lines to set, not the number of // lines in the request gpiod_line_request_set_values(req, num_lines, offsets, values); Bartosz > > Does this make sense? I'm worried about the resource management here. > > Who should be responsible for freeing the config structures? Should > > the extended config take ownership? Should the user remain > > responsible? Back to reference counting for these objects? Is this > > even a good idea? > > > > The user is responsible for freeing the request config. > The request config is responsible for freeing the line configs, if there > are any - the user isn't even aware of them so they obviously can't. > > Similarly if you merge the config into the request. > > > Please let me know what you think, I could use some advice. > > Hopefully I've communicated my meaning a little more clearly this time? > > Cheers, > Kent.
On Thu, Apr 22, 2021 at 11:24:34AM +0200, Bartosz Golaszewski wrote: > On Thu, Apr 22, 2021 at 4:32 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Apr 21, 2021 at 10:04:04PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Apr 19, 2021 at 3:17 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > [snip -> long discussion on the libgpiod C API] > > > > > > Hi Kent, > > > > > > I was working on the next iteration of the code and I'm struggling > > > with the implementation of some elements without the concept of > > > attributes. > > > > > > So I initially liked the idea of variadic functions but they won't > > > work for language bindings so that's a no go. On that note: I wanted > > > to get some inspiration from your go library but your elegant API > > > makes use of go features (like interfaces) we don't have in C. > > > > > > > It is the functional options that is the big difference between my Go > > implementation and what I would do in C. I happen to use interfaces to > > implement those options. You could do something similar in C (cos what > > can't you do in C) but it would be weird, so lets not go there. > > > > You can still provide the variadic forms for C users. > > And couldn't the language bindings use the "v" variant, as libabc > > suggests?: > > > > "Be careful with variadic functions > > - It's great if you provide them, but you must accompany them with > > "v" variants (i.e. functions taking a va_arg object), and provide > > non-variadic variants as well. This is important to get language > > wrappers right." > > > > The "v" functions do nothing for language bindings - you can't > construct the required va_list out of thin air. What you do usually is > this (example taken from GLib): > You are right - I was thinking you could build and pass in the the va_list like Python args and kwargs, but you can only use it to decode an existing call stack :-|. [snip] > > So it should require the list of lines that you are setting the values > > for. i.e. it is the mutator for a subset of lines, so the C case. > > > > And it implicitly sets those lines to outputs, so it can be more clearly > > named set_output(value) (that is the A case, btw). > > > > I can imagine the B case like: > > gpiod_line_config_set_output_value(config, offset, value); > > But how would exactly the call for the A case look like? Two arrays > with offset -> value mapping? > No - the A case sets ALL lines to one value. B is one line to one value. C is a set of lines to one value. A set of lines to a set of values is a new case. And yes - two arrays as per your set_values() below. > unsigned int offsets[] = { 0, 2, 5 }, values[] = { 0, 1 ,1 }; > gpiod_line_config_set_output_values_offsets(config, 3, offsets, values); > > ? > > > > One can imagine a simple request with the same config for all lines as: > > > > > > gpiod_chip_request_lines(chip, req_cfg, line_cfg); > > > > > > Where req_cfg configures request-specific options, and line_cfg > > > contains the above line config. I'm still not convinced that > > > gpiod_request_options is the better name, I think I prefer the > > > juxtaposition of the two names: line_config and request_config. > > > > > > > That's ok - I'm pretty sure you'll get there eventually ;-). > > > > > Now how do we pass a composite line config with overridden values in C > > > without interfaces etc.? > > > > > > > As above, the req_cfg is the composite line config, so > > > > req = gpiod_chip_request_lines(chip, req_options, req_cfg); > > > > Or if you were to merge the request config, and even the options, into the > > request: > > > > unsigned int lines[] = { 0, 4, 12, 54 }, num_lines = 4; > > req = gpiod_line_request_new(num_lines, lines); // also variadic forms?? > > // call req option and config mutators here... > > gpiod_line_request_set_active_low(req); > > gpiod_line_request_set_output(req, 1); > > gpiod_line_request_set_line_input(req, 12); > > gpiod_line_request_set_event_buffer_size(req, 42); > > ... > > // then actually request the lines... > > err = gpiod_chip_request_lines(chip, req); > > > > which may error for various reasons, such as lines already being > > requested or overly complex config. > > > > Merging everything into the request means fewer opaque objects and > > interactions for the user to have to deal with, which is always a good > > thing. > > The downside is that changes to options and config, such as the > > gpiod_line_request_set_active_low() etc here, are not applied until > > either the gpiod_chip_request_lines() or the set_config() call, which > > could be confusing. Though the more I think about it the more I think > > the resulting simplification of the API wins out. i.e. these objects: > > > > struct gpiod_line_attr; > > struct gpiod_line_config; > > struct gpiod_request_config; > > struct gpiod_request_handle; > > > > all get collapsed into: > > > > struct gpiod_line_request; > > > > which significantly reduces the cognitive load on the user. > > > > The set_config() would probably be called something like: > > > > err = gpiod_line_request_reconfigure(req) > > > > This lack of splitting of options into configurable and constant ones > visually suggests that you can change all request options later on > which is not true. Yup, as I said, the semantics for the unified object are more confusing. In the Go implementation, the request options can be passed to the request_lines(), but not the set_config(), cos interfaces. There is no good way to flag that in C at compile time. For a runtime check you could add a return code to the option mutators and return an error if the lines have already been requested. > I think that at least for the C API, we should > split the responsibilities of objects and keep the division into > request config, line config *and* the line handle whose lifetime is > from the moment the lines get requested until they're released. > > > to distinguish it from the mutators which use the _set_ naming. > > (and it would also align with my Go library ;-) > > > > > One idea I have is to add a new object called struct > > > gpiod_line_config_ext (for extended) that would take one primary > > > config and an arbitrary number of secondary configs with the following > > > example use-case: > > > > > > struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new(); > > > unsigned int offsets[] = { 2, 3 }; > > > > > > /* Add the default config for this request. */ > > > gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg); > > > /* Add a secondary config for 2 lines with offsets: 2 and 3. */ > > > gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets); > > > > > > gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg); > > > > > > > Please, no _ext objects - that is an admission of failure right there. > > > > I wanted to protest but then realized that if you need _ext interfaces > then it means your non-extended, initial design is already flawed. :) > > Ok so let's try again. > > How about: > > Three structs: > > struct gpiod_line_config; > struct gpiod_request_config; > struct gpiod_line_request; > The user manages the lifecycle of all three? I can live with that, though I would probably still lean towards the unified object approach - with the option mutators getting return codes. > The first one holds the composite of primary and secondary configs and > is modified using mutators according to this scheme: > Which is why it should be called request_config, not line_config. line_config is misleading - it says line but its scope is request. And of course request_config should be request_options ;-). > gpiod_line_config_set_<attr>(config, attr); > gpiod_line_config_set_<attr>_offset(config, attr, offset); > gpiod_line_config_set_<attr>_offsets(config, attr, num_offsets, offsets); > I personally prefer the _set_line_<attr> style as it reads better, but I can live with this - I know you prefer suffixes for variants. > With notable exceptions for: > > gpiod_line_config_set_[input|active_low|active_high](config); > gpiod_line_config_set_[input|active_low|active_high]_offset(config, offset); > gpiod_line_config_set_[input|active_low|active_high]_offsets(config, > num_offsets, offsets); > > and: > > gpiod_line_config_set_output(config, num_lines, offsets, values); > gpiod_line_config_set_output_offset(config, offset, value); > > The request function takes a single line config and a request config > and returns a new gpiod_line_request like in the first iteration. > Where are the set of requested lines specified? Do null config ptrs result in something useful, but guaranteed harmless, such as requesting lines as input? Or are they required non-null? > Then the lines can be set like this: > > // num_lines refers to the number of lines to set, not the number of > // lines in the request > gpiod_line_request_set_values(req, num_lines, offsets, values); > At first glance that feels a bit odd, being on the request while the others all operate on the config, but it maps to the SET_VALUES ioctl(), not the SET_CONFIG, so that makes sense. There is a corresponding get_values(req, num_lines, offsets, values)? And a reconfigure(req, cfg)? Cheers, Kent.
On Fri, Apr 23, 2021 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote: > [snip] > > > > So it should require the list of lines that you are setting the values > > > for. i.e. it is the mutator for a subset of lines, so the C case. > > > > > > And it implicitly sets those lines to outputs, so it can be more clearly > > > named set_output(value) (that is the A case, btw). > > > > > > > I can imagine the B case like: > > > > gpiod_line_config_set_output_value(config, offset, value); > > > > But how would exactly the call for the A case look like? Two arrays > > with offset -> value mapping? > > > > No - the A case sets ALL lines to one value. Apart from a single line request - what could possibly be the use-case for that? > B is one line to one value. > C is a set of lines to one value. This makes a bit more sense but ... > > A set of lines to a set of values is a new case. > And yes - two arrays as per your set_values() below. > ... to me this and B are the most sensible options. Do we really need A & C for line reading and setting? Do you find use for that in your Go lib? > > unsigned int offsets[] = { 0, 2, 5 }, values[] = { 0, 1 ,1 }; > > gpiod_line_config_set_output_values_offsets(config, 3, offsets, values); > > > > ? > > > > > > One can imagine a simple request with the same config for all lines as: > > > > > > > > gpiod_chip_request_lines(chip, req_cfg, line_cfg); > > > > > > > > Where req_cfg configures request-specific options, and line_cfg > > > > contains the above line config. I'm still not convinced that > > > > gpiod_request_options is the better name, I think I prefer the > > > > juxtaposition of the two names: line_config and request_config. > > > > > > > > > > That's ok - I'm pretty sure you'll get there eventually ;-). > > > > > > > Now how do we pass a composite line config with overridden values in C > > > > without interfaces etc.? > > > > > > > > > > As above, the req_cfg is the composite line config, so > > > > > > req = gpiod_chip_request_lines(chip, req_options, req_cfg); > > > > > > Or if you were to merge the request config, and even the options, into the > > > request: > > > > > > unsigned int lines[] = { 0, 4, 12, 54 }, num_lines = 4; > > > req = gpiod_line_request_new(num_lines, lines); // also variadic forms?? > > > // call req option and config mutators here... > > > gpiod_line_request_set_active_low(req); > > > gpiod_line_request_set_output(req, 1); > > > gpiod_line_request_set_line_input(req, 12); > > > gpiod_line_request_set_event_buffer_size(req, 42); > > > ... > > > // then actually request the lines... > > > err = gpiod_chip_request_lines(chip, req); > > > > > > which may error for various reasons, such as lines already being > > > requested or overly complex config. > > > > > > Merging everything into the request means fewer opaque objects and > > > interactions for the user to have to deal with, which is always a good > > > thing. > > > The downside is that changes to options and config, such as the > > > gpiod_line_request_set_active_low() etc here, are not applied until > > > either the gpiod_chip_request_lines() or the set_config() call, which > > > could be confusing. Though the more I think about it the more I think > > > the resulting simplification of the API wins out. i.e. these objects: > > > > > > struct gpiod_line_attr; > > > struct gpiod_line_config; > > > struct gpiod_request_config; > > > struct gpiod_request_handle; > > > > > > all get collapsed into: > > > > > > struct gpiod_line_request; > > > > > > which significantly reduces the cognitive load on the user. > > > > > > The set_config() would probably be called something like: > > > > > > err = gpiod_line_request_reconfigure(req) > > > > > > > This lack of splitting of options into configurable and constant ones > > visually suggests that you can change all request options later on > > which is not true. > > Yup, as I said, the semantics for the unified object are more confusing. > > In the Go implementation, the request options can be passed to the > request_lines(), but not the set_config(), cos interfaces. > > There is no good way to flag that in C at compile time. For a runtime > check you could add a return code to the option mutators and return an > error if the lines have already been requested. > I agree that it doesn't map well to C and this is why I think it would be less confusing if we went with two structs instead. > > I think that at least for the C API, we should > > split the responsibilities of objects and keep the division into > > request config, line config *and* the line handle whose lifetime is > > from the moment the lines get requested until they're released. > > > > > to distinguish it from the mutators which use the _set_ naming. > > > (and it would also align with my Go library ;-) > > > > > > > One idea I have is to add a new object called struct > > > > gpiod_line_config_ext (for extended) that would take one primary > > > > config and an arbitrary number of secondary configs with the following > > > > example use-case: > > > > > > > > struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new(); > > > > unsigned int offsets[] = { 2, 3 }; > > > > > > > > /* Add the default config for this request. */ > > > > gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg); > > > > /* Add a secondary config for 2 lines with offsets: 2 and 3. */ > > > > gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets); > > > > > > > > gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg); > > > > > > > > > > Please, no _ext objects - that is an admission of failure right there. > > > > > > > I wanted to protest but then realized that if you need _ext interfaces > > then it means your non-extended, initial design is already flawed. :) > > > > Ok so let's try again. > > > > How about: > > > > Three structs: > > > > struct gpiod_line_config; > > struct gpiod_request_config; > > struct gpiod_line_request; > > > > The user manages the lifecycle of all three? Yes. > > I can live with that, though I would probably still lean towards the > unified object approach - with the option mutators getting return codes. > > > The first one holds the composite of primary and secondary configs and > > is modified using mutators according to this scheme: > > > > Which is why it should be called request_config, not line_config. > line_config is misleading - it says line but its scope is request. > It depends on how you look at it really. Its scope are *the lines* in the request, not the request itself (unlike the event buffer size or line offsets). It says line because gpiod_lines_config would look bizarre. > And of course request_config should be request_options ;-). I'm still not there yet. > > > gpiod_line_config_set_<attr>(config, attr); > > gpiod_line_config_set_<attr>_offset(config, attr, offset); > > gpiod_line_config_set_<attr>_offsets(config, attr, num_offsets, offsets); > > > > I personally prefer the _set_line_<attr> style as it reads better, but I > can live with this - I know you prefer suffixes for variants. > > > With notable exceptions for: > > > > gpiod_line_config_set_[input|active_low|active_high](config); > > gpiod_line_config_set_[input|active_low|active_high]_offset(config, offset); > > gpiod_line_config_set_[input|active_low|active_high]_offsets(config, > > num_offsets, offsets); > > > > and: > > > > gpiod_line_config_set_output(config, num_lines, offsets, values); > > gpiod_line_config_set_output_offset(config, offset, value); > > > > The request function takes a single line config and a request config > > and returns a new gpiod_line_request like in the first iteration. > > > > Where are the set of requested lines specified? > They map the uAPI in that the offsets are set in struct gpiod_request_config: gpiod_request_config_set_line_offsets(config, num_lines, offsets); :) or gpiod_request_options_set_line_offsets(config, num_lines, offsets); :( > Do null config ptrs result in something useful, but guaranteed harmless, > such as requesting lines as input? Or are they required non-null? > I normally expect users to pass a valid pointer and don't make the functions null aware. In this case - it's a good question. I'm wondering if it is useful at all? The user should IMO specify what they want to do with the lines? I would lean towards non-null line configfs. > > Then the lines can be set like this: > > > > // num_lines refers to the number of lines to set, not the number of > > // lines in the request > > gpiod_line_request_set_values(req, num_lines, offsets, values); > > > > At first glance that feels a bit odd, being on the request while the > others all operate on the config, but it maps to the SET_VALUES ioctl(), > not the SET_CONFIG, so that makes sense. > > There is a corresponding get_values(req, num_lines, offsets, values)? > Sure, just didn't include it in the example. > And a reconfigure(req, cfg)? > Sure, just haven't decided on the name yet. gpiod_line_request_reconfigure() would be what you're suggesting but it sounds like it would reconfigure the request and not modify the line configuration. I would prefer something closer to gpiod_line_request_set_line_config() which is as verbose as it gets. Maybe even gpiod_line_request_change_line_config()? Or gpiod_line_request_modify_line_config()? Bartosz > Cheers, > Kent. >
On Wed, Apr 28, 2021 at 11:19:05AM +0200, Bartosz Golaszewski wrote: > On Fri, Apr 23, 2021 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip] > > > > > > > So it should require the list of lines that you are setting the values > > > > for. i.e. it is the mutator for a subset of lines, so the C case. > > > > > > > > And it implicitly sets those lines to outputs, so it can be more clearly > > > > named set_output(value) (that is the A case, btw). > > > > > > > > > > I can imagine the B case like: > > > > > > gpiod_line_config_set_output_value(config, offset, value); > > > > > > But how would exactly the call for the A case look like? Two arrays > > > with offset -> value mapping? > > > > > > > No - the A case sets ALL lines to one value. > > Apart from a single line request - what could possibly be the use-case for that? > The A, B, C nomenclature originated for attributes, for which blanket sets (A) make more sense. The case for using A for outputs would be if the vast majority of your lines default to one value. You would then only have to use the other (B, C or whatever) for the non-default lines. > > B is one line to one value. > > C is a set of lines to one value. > > This makes a bit more sense but ... > > > > > A set of lines to a set of values is a new case. > > And yes - two arrays as per your set_values() below. > > For future reference, lets call this case D. > > ... to me this and B are the most sensible options. Do we really need > A & C for line reading and setting? Do you find use for that in your > Go lib? > No need for all four - two would be sufficient - probably B and D as you suggest. The Go interface is a little different because it is being passed as options. It doesn't provide all four. It provides D. And setting all offsets in the request to specific values as part of the request contructor - similar to the set in your initial draft. And it defaults lines to inactive if not specified otherwise. For gets and sets it only supports all the requested lines. Strictly speaking it can request a subset of the lines - but only the first n lines not a sparse subset. It could be extended to the D case, but so far no one has asked. > > > unsigned int offsets[] = { 0, 2, 5 }, values[] = { 0, 1 ,1 }; > > > gpiod_line_config_set_output_values_offsets(config, 3, offsets, values); > > > > > > ? > > > > > > > > One can imagine a simple request with the same config for all lines as: > > > > > > > > > > gpiod_chip_request_lines(chip, req_cfg, line_cfg); > > > > > > > > > > Where req_cfg configures request-specific options, and line_cfg > > > > > contains the above line config. I'm still not convinced that > > > > > gpiod_request_options is the better name, I think I prefer the > > > > > juxtaposition of the two names: line_config and request_config. > > > > > > > > > > > > > That's ok - I'm pretty sure you'll get there eventually ;-). > > > > > > > > > Now how do we pass a composite line config with overridden values in C > > > > > without interfaces etc.? > > > > > > > > > > > > > As above, the req_cfg is the composite line config, so > > > > > > > > req = gpiod_chip_request_lines(chip, req_options, req_cfg); > > > > > > > > Or if you were to merge the request config, and even the options, into the > > > > request: > > > > > > > > unsigned int lines[] = { 0, 4, 12, 54 }, num_lines = 4; > > > > req = gpiod_line_request_new(num_lines, lines); // also variadic forms?? > > > > // call req option and config mutators here... > > > > gpiod_line_request_set_active_low(req); > > > > gpiod_line_request_set_output(req, 1); > > > > gpiod_line_request_set_line_input(req, 12); > > > > gpiod_line_request_set_event_buffer_size(req, 42); > > > > ... > > > > // then actually request the lines... > > > > err = gpiod_chip_request_lines(chip, req); > > > > > > > > which may error for various reasons, such as lines already being > > > > requested or overly complex config. > > > > > > > > Merging everything into the request means fewer opaque objects and > > > > interactions for the user to have to deal with, which is always a good > > > > thing. > > > > The downside is that changes to options and config, such as the > > > > gpiod_line_request_set_active_low() etc here, are not applied until > > > > either the gpiod_chip_request_lines() or the set_config() call, which > > > > could be confusing. Though the more I think about it the more I think > > > > the resulting simplification of the API wins out. i.e. these objects: > > > > > > > > struct gpiod_line_attr; > > > > struct gpiod_line_config; > > > > struct gpiod_request_config; > > > > struct gpiod_request_handle; > > > > > > > > all get collapsed into: > > > > > > > > struct gpiod_line_request; > > > > > > > > which significantly reduces the cognitive load on the user. > > > > > > > > The set_config() would probably be called something like: > > > > > > > > err = gpiod_line_request_reconfigure(req) > > > > > > > > > > This lack of splitting of options into configurable and constant ones > > > visually suggests that you can change all request options later on > > > which is not true. > > > > Yup, as I said, the semantics for the unified object are more confusing. > > > > In the Go implementation, the request options can be passed to the > > request_lines(), but not the set_config(), cos interfaces. > > > > There is no good way to flag that in C at compile time. For a runtime > > check you could add a return code to the option mutators and return an > > error if the lines have already been requested. > > > > I agree that it doesn't map well to C and this is why I think it would > be less confusing if we went with two structs instead. > I know. My concern is that the simplest get use case is 7 function calls: 1. create request_config 2. create line_config 3. request lines 4. do the actual work 5. release request 6. free line_config 7. free request_config And that is ignoring the chip open/close functions. > > > I think that at least for the C API, we should > > > split the responsibilities of objects and keep the division into > > > request config, line config *and* the line handle whose lifetime is > > > from the moment the lines get requested until they're released. > > > > > > > to distinguish it from the mutators which use the _set_ naming. > > > > (and it would also align with my Go library ;-) > > > > > > > > > One idea I have is to add a new object called struct > > > > > gpiod_line_config_ext (for extended) that would take one primary > > > > > config and an arbitrary number of secondary configs with the following > > > > > example use-case: > > > > > > > > > > struct gpiod_line_config_ext *ext_cfg = gpiod_line_config_ext_new(); > > > > > unsigned int offsets[] = { 2, 3 }; > > > > > > > > > > /* Add the default config for this request. */ > > > > > gpiod_line_config_ext_set_primary_config(ext_cfg, line_cfg); > > > > > /* Add a secondary config for 2 lines with offsets: 2 and 3. */ > > > > > gpiod_line_config_ext_add_secondary_config(ext_cfg, other_line_cfg, 2, offsets); > > > > > > > > > > gpiod_chip_request_lines_ext(chip, req_cfg, ext_cfg); > > > > > > > > > > > > > Please, no _ext objects - that is an admission of failure right there. > > > > > > > > > > I wanted to protest but then realized that if you need _ext interfaces > > > then it means your non-extended, initial design is already flawed. :) > > > > > > Ok so let's try again. > > > > > > How about: > > > > > > Three structs: > > > > > > struct gpiod_line_config; > > > struct gpiod_request_config; > > > struct gpiod_line_request; > > > > > > > The user manages the lifecycle of all three? > > Yes. > > > > > I can live with that, though I would probably still lean towards the > > unified object approach - with the option mutators getting return codes. > > > > > The first one holds the composite of primary and secondary configs and > > > is modified using mutators according to this scheme: > > > > > > > Which is why it should be called request_config, not line_config. > > line_config is misleading - it says line but its scope is request. > > > > It depends on how you look at it really. Its scope are *the lines* in > the request, not the request itself (unlike the event buffer size or > line offsets). It says line because gpiod_lines_config would look > bizarre. > > > And of course request_config should be request_options ;-). > > I'm still not there yet. > > > > > > gpiod_line_config_set_<attr>(config, attr); > > > gpiod_line_config_set_<attr>_offset(config, attr, offset); > > > gpiod_line_config_set_<attr>_offsets(config, attr, num_offsets, offsets); > > > > > > > I personally prefer the _set_line_<attr> style as it reads better, but I > > can live with this - I know you prefer suffixes for variants. > > > > > With notable exceptions for: > > > > > > gpiod_line_config_set_[input|active_low|active_high](config); > > > gpiod_line_config_set_[input|active_low|active_high]_offset(config, offset); > > > gpiod_line_config_set_[input|active_low|active_high]_offsets(config, > > > num_offsets, offsets); > > > > > > and: > > > > > > gpiod_line_config_set_output(config, num_lines, offsets, values); > > > gpiod_line_config_set_output_offset(config, offset, value); > > > > > > The request function takes a single line config and a request config > > > and returns a new gpiod_line_request like in the first iteration. > > > > > > > Where are the set of requested lines specified? > > > > They map the uAPI in that the offsets are set in struct gpiod_request_config: > > gpiod_request_config_set_line_offsets(config, num_lines, offsets); :) > > or > > gpiod_request_options_set_line_offsets(config, num_lines, offsets); :( > Alternatively it could be in one of the constructors - gpiod_request_config_new(), gpiod_line_config_new() or gpiod_chip_request_lines()? > > Do null config ptrs result in something useful, but guaranteed harmless, > > such as requesting lines as input? Or are they required non-null? > > > > I normally expect users to pass a valid pointer and don't make the > functions null aware. In this case - it's a good question. I'm > wondering if it is useful at all? The user should IMO specify what > they want to do with the lines? I would lean towards non-null line > configfs. > Sure doesn't if you have to set the offsets in the config. But might do if you provide them to the gpiod_chip_request_lines(). The null case could then request the lines as-is? There seems to be a bit of interest in as-is of late, and the simplest case would then be three function calls. > > > Then the lines can be set like this: > > > > > > // num_lines refers to the number of lines to set, not the number of > > > // lines in the request > > > gpiod_line_request_set_values(req, num_lines, offsets, values); > > > > > > > At first glance that feels a bit odd, being on the request while the > > others all operate on the config, but it maps to the SET_VALUES ioctl(), > > not the SET_CONFIG, so that makes sense. > > > > There is a corresponding get_values(req, num_lines, offsets, values)? > > > > Sure, just didn't include it in the example. > > > And a reconfigure(req, cfg)? > > > > Sure, just haven't decided on the name yet. > gpiod_line_request_reconfigure() would be what you're suggesting but > it sounds like it would reconfigure the request and not modify the > line configuration. I would prefer something closer to > gpiod_line_request_set_line_config() which is as verbose as it gets. > Maybe even gpiod_line_request_change_line_config()? Or > gpiod_line_request_modify_line_config()? > gpiod_line_request_reconfigure(req, cfg) is not modifying the line_config - it is modifying the request WITH the modified config. If gpiod_line_request_<splat>_line_config(req, cfg) works better for you then I'd prefer something along the lines of _apply_ or _commit_, as it is taking the line_config, modified by sets, and applying it to the requested lines. Cheers, Kent.
On Wed, Apr 28, 2021 at 12:34 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Apr 28, 2021 at 11:19:05AM +0200, Bartosz Golaszewski wrote: > > On Fri, Apr 23, 2021 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > [snip] > > > > > > > > > > So it should require the list of lines that you are setting the values > > > > > for. i.e. it is the mutator for a subset of lines, so the C case. > > > > > > > > > > And it implicitly sets those lines to outputs, so it can be more clearly > > > > > named set_output(value) (that is the A case, btw). > > > > > > > > > > > > > I can imagine the B case like: > > > > > > > > gpiod_line_config_set_output_value(config, offset, value); > > > > > > > > But how would exactly the call for the A case look like? Two arrays > > > > with offset -> value mapping? > > > > > > > > > > No - the A case sets ALL lines to one value. > > > > Apart from a single line request - what could possibly be the use-case for that? > > > > The A, B, C nomenclature originated for attributes, for which blanket > sets (A) make more sense. > > The case for using A for outputs would be if the vast majority of your > lines default to one value. You would then only have to use the other > (B, C or whatever) for the non-default lines. > > > > B is one line to one value. > > > C is a set of lines to one value. > > > > This makes a bit more sense but ... > > > > > > > > A set of lines to a set of values is a new case. > > > And yes - two arrays as per your set_values() below. > > > > > For future reference, lets call this case D. > > > > > ... to me this and B are the most sensible options. Do we really need > > A & C for line reading and setting? Do you find use for that in your > > Go lib? > > > > No need for all four - two would be sufficient - probably B and D as you > suggest. > Agreed. [snip] > > > > > > > > This lack of splitting of options into configurable and constant ones > > > > visually suggests that you can change all request options later on > > > > which is not true. > > > > > > Yup, as I said, the semantics for the unified object are more confusing. > > > > > > In the Go implementation, the request options can be passed to the > > > request_lines(), but not the set_config(), cos interfaces. > > > > > > There is no good way to flag that in C at compile time. For a runtime > > > check you could add a return code to the option mutators and return an > > > error if the lines have already been requested. > > > > > > > I agree that it doesn't map well to C and this is why I think it would > > be less confusing if we went with two structs instead. > > > > I know. > > My concern is that the simplest get use case is 7 function calls: > 1. create request_config > 2. create line_config > 3. request lines > 4. do the actual work > 5. release request > 6. free line_config > 7. free request_config > > And that is ignoring the chip open/close functions. > This doesn't bother me all that much. This is the low-level C API. It's supposed to be a bit clunky. It'll get way simpler in the high-level bindings. In Python a simple case would look something like this: with gpiod.Chip("/dev/gpiochip0") as chip: with chip.request_lines(gpiod.RequestConfig(offsets=( 0, 2, 3 )), gpiod.LineConfig(direction=gpiod.DIRECTION_INPUT)) as req: values = req.get_values() # Read all values values = req.get_values(offsets=( 2, 3)) # Read values of specific lines [snip] > > > > > > Where are the set of requested lines specified? > > > > > > > They map the uAPI in that the offsets are set in struct gpiod_request_config: > > > > gpiod_request_config_set_line_offsets(config, num_lines, offsets); :) > > > > or > > > > gpiod_request_options_set_line_offsets(config, num_lines, offsets); :( > > > > Alternatively it could be in one of the constructors - > gpiod_request_config_new(), gpiod_line_config_new() or > gpiod_chip_request_lines()? > I want to make the struct reusable/modifiable so setting it only in the constructor would disallow it and having both would be redundant. And the last one: the less function arguments the better IMO. So I'm for having it in the request_config. > > > Do null config ptrs result in something useful, but guaranteed harmless, > > > such as requesting lines as input? Or are they required non-null? > > > > > > > I normally expect users to pass a valid pointer and don't make the > > functions null aware. In this case - it's a good question. I'm > > wondering if it is useful at all? The user should IMO specify what > > they want to do with the lines? I would lean towards non-null line > > configfs. > > > > Sure doesn't if you have to set the offsets in the config. > But might do if you provide them to the gpiod_chip_request_lines(). > > The null case could then request the lines as-is? > > There seems to be a bit of interest in as-is of late, and the simplest > case would then be three function calls. > Ok, makes sense. Null line-config -> request lines as is. > > > > Then the lines can be set like this: > > > > > > > > // num_lines refers to the number of lines to set, not the number of > > > > // lines in the request > > > > gpiod_line_request_set_values(req, num_lines, offsets, values); > > > > > > > > > > At first glance that feels a bit odd, being on the request while the > > > others all operate on the config, but it maps to the SET_VALUES ioctl(), > > > not the SET_CONFIG, so that makes sense. > > > > > > There is a corresponding get_values(req, num_lines, offsets, values)? > > > > > > > Sure, just didn't include it in the example. > > > > > And a reconfigure(req, cfg)? > > > > > > > Sure, just haven't decided on the name yet. > > gpiod_line_request_reconfigure() would be what you're suggesting but > > it sounds like it would reconfigure the request and not modify the > > line configuration. I would prefer something closer to > > gpiod_line_request_set_line_config() which is as verbose as it gets. > > Maybe even gpiod_line_request_change_line_config()? Or > > gpiod_line_request_modify_line_config()? > > > > gpiod_line_request_reconfigure(req, cfg) is not modifying the line_config > - it is modifying the request WITH the modified config. > > If gpiod_line_request_<splat>_line_config(req, cfg) works better for you > then I'd prefer something along the lines of _apply_ or _commit_, as it is > taking the line_config, modified by sets, and applying it to the > requested lines. > How about gpiod_line_request_reconfigure_lines()? Bart
On Fri, Apr 30, 2021 at 07:52:05PM +0200, Bartosz Golaszewski wrote: > On Wed, Apr 28, 2021 at 12:34 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Apr 28, 2021 at 11:19:05AM +0200, Bartosz Golaszewski wrote: > > > On Fri, Apr 23, 2021 at 3:39 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > [snip] > > > > If gpiod_line_request_<splat>_line_config(req, cfg) works better for you > > then I'd prefer something along the lines of _apply_ or _commit_, as it is > > taking the line_config, modified by sets, and applying it to the > > requested lines. > > > > How about gpiod_line_request_reconfigure_lines()? > That works for me. Cheers, Kent.