Re: ADC device not allowed multiple opens
Thanks for all the input. I will look at the adc driver in a bit more detail to see which option is best. My original thought was to do what you had done Andrzej: if the device is already opened, just return OK. But I will look at the various locks and such to determine if there will be issues here. Thanks all > On Jun 6, 2018, at 3:17 PM, Andrzej Kaczmarek > wrote: > > Hi, > > +1 for proper ref counting > btw, trng_nrf52 already works this way except it does not check ref > counter explicitly but OS_DEV_F_STATUS_OPEN flag instead. This is also > already used in Mynewt as NimBLE host, controller and TinyCrypt can > (depending on configuration) just open trng device on its own and > there is no need to open it "globally". > > One thing that may need to be fixed though is some locking when > calling open/close handlers since they are now called unlocked and ref > counter is updated later. Since ref counter is checked in these > handlers this may cause device to be initialized more than once and > also to be not un-initialized when all references are dropped. Both > are not a problem for trng_nrf52 driver so I did not worry about it > too much ;-) > > Best, > Andrzej > > > On Wed, Jun 6, 2018 at 10:56 PM will sanfilippo wrote: >> >> Chris and Vipul: >> >> That does indeed seem to be a decent option! I will see if others chime in >> and if not, add that change. >> >> Thanks!!! >> >>> On Jun 6, 2018, at 1:32 PM, Vipul Rahane wrote: >>> >>> On Jun 6, 2018, at 1:01 PM, Christopher Collins wrote: On Wed, Jun 06, 2018 at 11:57:34AM -0700, will sanfilippo wrote: > Chris: > > I might be missing something here, but given that os_dev already has a > reference count and that handles multiple folks opening/closing the > device, does the underlying adc driver need a reference count itself? If > it just returned no error if opened again this would be fine. > > I do note that os_dev_open() and os_dev_close() always call the > open/close handlers regardless of reference count. I wonder if that > should be changed (meaning only call open/close once)? No, you aren't missing anything; I just misunderstood the os_dev reference counting. Thanks for setting me straight :). Another option: the ADC open function checks its os_dev's reference count. If the value is greater than zero, then return without doing anything. >>> >>> I was going to suggest that as well. Seems like a simple solution :-) >>> Chris > > >> On Jun 6, 2018, at 10:13 AM, Christopher Collins >> wrote: >> >> On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: >>> Hello: >>> >>> I am not the most familiar with the ADC device so it is possible that >>> it was being used incorrectly but in any event I ran across something I >>> wanted to discuss. The call to os_dev_open() allows a device to be >>> opened multiple times (there is a reference count there). However, the >>> call to nrf52_adc_open() returns an error (OS_EBUSY) if the device has >>> already been opened. >>> >>> This presented a problem in the following case: consider two >>> independent packages both of which want to use ADC_0. Each package is >>> going to attempt to open the ADC device (since it has no idea if it was >>> already opened) but the second attempt to open the device will result >>> in an error code returned. Depending on how the code is written in the >>> package, this could be a problem. Given that an ADC is almost always a >>> mutli-channel peripheral (one adc device has multple channels) I would >>> suspect the above case to be common: multiple packages wanting an ADC >>> channel from a single device. >>> >>> I am not sure if anything needs to be done here; just wanted to see if >>> folks thought there should different behavior with regards to the >>> function returning an error if the device was already opened. If not, >>> folks are going to have to be careful when they write code using the >>> adc device. Seems to me if nothing is going to change we have two >>> options: >>> >>> 1) The device gets created and opened in some place and handed to the >>> packages that need it. >>> 2) The device gets created (say by the bsp) and each package can >>> attempt to open the device. If os_dev_lookup() returns !NULL but >>> os_dev_open() returns NULL it means that the device has already been >>> opened. >>> >>> Something about #2 just sort of bothers me. I do not like ambiguous >>> stuff like that; how do you know if there was an error for another >>> reason? >> >> Why not: >> >> 3) Make the ADC driver consistent with other drivers by adding a >> reference count. >> >> ? >> >> I know something less than
Re: ADC device not allowed multiple opens
Hi, +1 for proper ref counting btw, trng_nrf52 already works this way except it does not check ref counter explicitly but OS_DEV_F_STATUS_OPEN flag instead. This is also already used in Mynewt as NimBLE host, controller and TinyCrypt can (depending on configuration) just open trng device on its own and there is no need to open it "globally". One thing that may need to be fixed though is some locking when calling open/close handlers since they are now called unlocked and ref counter is updated later. Since ref counter is checked in these handlers this may cause device to be initialized more than once and also to be not un-initialized when all references are dropped. Both are not a problem for trng_nrf52 driver so I did not worry about it too much ;-) Best, Andrzej On Wed, Jun 6, 2018 at 10:56 PM will sanfilippo wrote: > > Chris and Vipul: > > That does indeed seem to be a decent option! I will see if others chime in > and if not, add that change. > > Thanks!!! > > > On Jun 6, 2018, at 1:32 PM, Vipul Rahane wrote: > > > > > >> On Jun 6, 2018, at 1:01 PM, Christopher Collins wrote: > >> > >> On Wed, Jun 06, 2018 at 11:57:34AM -0700, will sanfilippo wrote: > >>> Chris: > >>> > >>> I might be missing something here, but given that os_dev already has a > >>> reference count and that handles multiple folks opening/closing the > >>> device, does the underlying adc driver need a reference count itself? If > >>> it just returned no error if opened again this would be fine. > >>> > >>> I do note that os_dev_open() and os_dev_close() always call the > >>> open/close handlers regardless of reference count. I wonder if that > >>> should be changed (meaning only call open/close once)? > >> > >> No, you aren't missing anything; I just misunderstood the os_dev > >> reference counting. Thanks for setting me straight :). > >> > >> Another option: the ADC open function checks its os_dev's reference > >> count. If the value is greater than zero, then return without doing > >> anything. > >> > > > > I was going to suggest that as well. Seems like a simple solution :-) > > > >> Chris > >> > >>> > >>> > On Jun 6, 2018, at 10:13 AM, Christopher Collins > wrote: > > On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: > > Hello: > > > > I am not the most familiar with the ADC device so it is possible that > > it was being used incorrectly but in any event I ran across something I > > wanted to discuss. The call to os_dev_open() allows a device to be > > opened multiple times (there is a reference count there). However, the > > call to nrf52_adc_open() returns an error (OS_EBUSY) if the device has > > already been opened. > > > > This presented a problem in the following case: consider two > > independent packages both of which want to use ADC_0. Each package is > > going to attempt to open the ADC device (since it has no idea if it was > > already opened) but the second attempt to open the device will result > > in an error code returned. Depending on how the code is written in the > > package, this could be a problem. Given that an ADC is almost always a > > mutli-channel peripheral (one adc device has multple channels) I would > > suspect the above case to be common: multiple packages wanting an ADC > > channel from a single device. > > > > I am not sure if anything needs to be done here; just wanted to see if > > folks thought there should different behavior with regards to the > > function returning an error if the device was already opened. If not, > > folks are going to have to be careful when they write code using the > > adc device. Seems to me if nothing is going to change we have two > > options: > > > > 1) The device gets created and opened in some place and handed to the > > packages that need it. > > 2) The device gets created (say by the bsp) and each package can > > attempt to open the device. If os_dev_lookup() returns !NULL but > > os_dev_open() returns NULL it means that the device has already been > > opened. > > > > Something about #2 just sort of bothers me. I do not like ambiguous > > stuff like that; how do you know if there was an error for another > > reason? > > Why not: > > 3) Make the ADC driver consistent with other drivers by adding a > reference count. > > ? > > I know something less than nothing about the ADC code, so I could > certainly be missing something. > > Chris > >>> > > > > - Vipul >
Re: ADC device not allowed multiple opens
Chris and Vipul: That does indeed seem to be a decent option! I will see if others chime in and if not, add that change. Thanks!!! > On Jun 6, 2018, at 1:32 PM, Vipul Rahane wrote: > > >> On Jun 6, 2018, at 1:01 PM, Christopher Collins wrote: >> >> On Wed, Jun 06, 2018 at 11:57:34AM -0700, will sanfilippo wrote: >>> Chris: >>> >>> I might be missing something here, but given that os_dev already has a >>> reference count and that handles multiple folks opening/closing the device, >>> does the underlying adc driver need a reference count itself? If it just >>> returned no error if opened again this would be fine. >>> >>> I do note that os_dev_open() and os_dev_close() always call the open/close >>> handlers regardless of reference count. I wonder if that should be changed >>> (meaning only call open/close once)? >> >> No, you aren't missing anything; I just misunderstood the os_dev >> reference counting. Thanks for setting me straight :). >> >> Another option: the ADC open function checks its os_dev's reference >> count. If the value is greater than zero, then return without doing >> anything. >> > > I was going to suggest that as well. Seems like a simple solution :-) > >> Chris >> >>> >>> On Jun 6, 2018, at 10:13 AM, Christopher Collins wrote: On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: > Hello: > > I am not the most familiar with the ADC device so it is possible that it > was being used incorrectly but in any event I ran across something I > wanted to discuss. The call to os_dev_open() allows a device to be opened > multiple times (there is a reference count there). However, the call to > nrf52_adc_open() returns an error (OS_EBUSY) if the device has already > been opened. > > This presented a problem in the following case: consider two independent > packages both of which want to use ADC_0. Each package is going to > attempt to open the ADC device (since it has no idea if it was already > opened) but the second attempt to open the device will result in an error > code returned. Depending on how the code is written in the package, this > could be a problem. Given that an ADC is almost always a mutli-channel > peripheral (one adc device has multple channels) I would suspect the > above case to be common: multiple packages wanting an ADC channel from a > single device. > > I am not sure if anything needs to be done here; just wanted to see if > folks thought there should different behavior with regards to the > function returning an error if the device was already opened. If not, > folks are going to have to be careful when they write code using the adc > device. Seems to me if nothing is going to change we have two options: > > 1) The device gets created and opened in some place and handed to the > packages that need it. > 2) The device gets created (say by the bsp) and each package can attempt > to open the device. If os_dev_lookup() returns !NULL but os_dev_open() > returns NULL it means that the device has already been opened. > > Something about #2 just sort of bothers me. I do not like ambiguous stuff > like that; how do you know if there was an error for another reason? Why not: 3) Make the ADC driver consistent with other drivers by adding a reference count. ? I know something less than nothing about the ADC code, so I could certainly be missing something. Chris >>> > > - Vipul
Re: ADC device not allowed multiple opens
> On Jun 6, 2018, at 1:01 PM, Christopher Collins wrote: > > On Wed, Jun 06, 2018 at 11:57:34AM -0700, will sanfilippo wrote: >> Chris: >> >> I might be missing something here, but given that os_dev already has a >> reference count and that handles multiple folks opening/closing the device, >> does the underlying adc driver need a reference count itself? If it just >> returned no error if opened again this would be fine. >> >> I do note that os_dev_open() and os_dev_close() always call the open/close >> handlers regardless of reference count. I wonder if that should be changed >> (meaning only call open/close once)? > > No, you aren't missing anything; I just misunderstood the os_dev > reference counting. Thanks for setting me straight :). > > Another option: the ADC open function checks its os_dev's reference > count. If the value is greater than zero, then return without doing > anything. > I was going to suggest that as well. Seems like a simple solution :-) > Chris > >> >> >>> On Jun 6, 2018, at 10:13 AM, Christopher Collins wrote: >>> >>> On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: Hello: I am not the most familiar with the ADC device so it is possible that it was being used incorrectly but in any event I ran across something I wanted to discuss. The call to os_dev_open() allows a device to be opened multiple times (there is a reference count there). However, the call to nrf52_adc_open() returns an error (OS_EBUSY) if the device has already been opened. This presented a problem in the following case: consider two independent packages both of which want to use ADC_0. Each package is going to attempt to open the ADC device (since it has no idea if it was already opened) but the second attempt to open the device will result in an error code returned. Depending on how the code is written in the package, this could be a problem. Given that an ADC is almost always a mutli-channel peripheral (one adc device has multple channels) I would suspect the above case to be common: multiple packages wanting an ADC channel from a single device. I am not sure if anything needs to be done here; just wanted to see if folks thought there should different behavior with regards to the function returning an error if the device was already opened. If not, folks are going to have to be careful when they write code using the adc device. Seems to me if nothing is going to change we have two options: 1) The device gets created and opened in some place and handed to the packages that need it. 2) The device gets created (say by the bsp) and each package can attempt to open the device. If os_dev_lookup() returns !NULL but os_dev_open() returns NULL it means that the device has already been opened. Something about #2 just sort of bothers me. I do not like ambiguous stuff like that; how do you know if there was an error for another reason? >>> >>> Why not: >>> >>> 3) Make the ADC driver consistent with other drivers by adding a >>> reference count. >>> >>> ? >>> >>> I know something less than nothing about the ADC code, so I could >>> certainly be missing something. >>> >>> Chris >> - Vipul
Re: ADC device not allowed multiple opens
On Wed, Jun 06, 2018 at 11:57:34AM -0700, will sanfilippo wrote: > Chris: > > I might be missing something here, but given that os_dev already has a > reference count and that handles multiple folks opening/closing the device, > does the underlying adc driver need a reference count itself? If it just > returned no error if opened again this would be fine. > > I do note that os_dev_open() and os_dev_close() always call the open/close > handlers regardless of reference count. I wonder if that should be changed > (meaning only call open/close once)? No, you aren't missing anything; I just misunderstood the os_dev reference counting. Thanks for setting me straight :). Another option: the ADC open function checks its os_dev's reference count. If the value is greater than zero, then return without doing anything. Chris > > > > On Jun 6, 2018, at 10:13 AM, Christopher Collins wrote: > > > > On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: > >> Hello: > >> > >> I am not the most familiar with the ADC device so it is possible that it > >> was being used incorrectly but in any event I ran across something I > >> wanted to discuss. The call to os_dev_open() allows a device to be opened > >> multiple times (there is a reference count there). However, the call to > >> nrf52_adc_open() returns an error (OS_EBUSY) if the device has already > >> been opened. > >> > >> This presented a problem in the following case: consider two independent > >> packages both of which want to use ADC_0. Each package is going to attempt > >> to open the ADC device (since it has no idea if it was already opened) but > >> the second attempt to open the device will result in an error code > >> returned. Depending on how the code is written in the package, this could > >> be a problem. Given that an ADC is almost always a mutli-channel > >> peripheral (one adc device has multple channels) I would suspect the above > >> case to be common: multiple packages wanting an ADC channel from a single > >> device. > >> > >> I am not sure if anything needs to be done here; just wanted to see if > >> folks thought there should different behavior with regards to the function > >> returning an error if the device was already opened. If not, folks are > >> going to have to be careful when they write code using the adc device. > >> Seems to me if nothing is going to change we have two options: > >> > >> 1) The device gets created and opened in some place and handed to the > >> packages that need it. > >> 2) The device gets created (say by the bsp) and each package can attempt > >> to open the device. If os_dev_lookup() returns !NULL but os_dev_open() > >> returns NULL it means that the device has already been opened. > >> > >> Something about #2 just sort of bothers me. I do not like ambiguous stuff > >> like that; how do you know if there was an error for another reason? > > > > Why not: > > > > 3) Make the ADC driver consistent with other drivers by adding a > > reference count. > > > > ? > > > > I know something less than nothing about the ADC code, so I could > > certainly be missing something. > > > > Chris >
Re: ADC device not allowed multiple opens
Chris: I might be missing something here, but given that os_dev already has a reference count and that handles multiple folks opening/closing the device, does the underlying adc driver need a reference count itself? If it just returned no error if opened again this would be fine. I do note that os_dev_open() and os_dev_close() always call the open/close handlers regardless of reference count. I wonder if that should be changed (meaning only call open/close once)? > On Jun 6, 2018, at 10:13 AM, Christopher Collins wrote: > > On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: >> Hello: >> >> I am not the most familiar with the ADC device so it is possible that it was >> being used incorrectly but in any event I ran across something I wanted to >> discuss. The call to os_dev_open() allows a device to be opened multiple >> times (there is a reference count there). However, the call to >> nrf52_adc_open() returns an error (OS_EBUSY) if the device has already been >> opened. >> >> This presented a problem in the following case: consider two independent >> packages both of which want to use ADC_0. Each package is going to attempt >> to open the ADC device (since it has no idea if it was already opened) but >> the second attempt to open the device will result in an error code returned. >> Depending on how the code is written in the package, this could be a >> problem. Given that an ADC is almost always a mutli-channel peripheral (one >> adc device has multple channels) I would suspect the above case to be >> common: multiple packages wanting an ADC channel from a single device. >> >> I am not sure if anything needs to be done here; just wanted to see if folks >> thought there should different behavior with regards to the function >> returning an error if the device was already opened. If not, folks are going >> to have to be careful when they write code using the adc device. Seems to me >> if nothing is going to change we have two options: >> >> 1) The device gets created and opened in some place and handed to the >> packages that need it. >> 2) The device gets created (say by the bsp) and each package can attempt to >> open the device. If os_dev_lookup() returns !NULL but os_dev_open() returns >> NULL it means that the device has already been opened. >> >> Something about #2 just sort of bothers me. I do not like ambiguous stuff >> like that; how do you know if there was an error for another reason? > > Why not: > > 3) Make the ADC driver consistent with other drivers by adding a > reference count. > > ? > > I know something less than nothing about the ADC code, so I could > certainly be missing something. > > Chris
Re: ADC device not allowed multiple opens
On Wed, Jun 06, 2018 at 08:50:34AM -0700, will sanfilippo wrote: > Hello: > > I am not the most familiar with the ADC device so it is possible that it was > being used incorrectly but in any event I ran across something I wanted to > discuss. The call to os_dev_open() allows a device to be opened multiple > times (there is a reference count there). However, the call to > nrf52_adc_open() returns an error (OS_EBUSY) if the device has already been > opened. > > This presented a problem in the following case: consider two independent > packages both of which want to use ADC_0. Each package is going to attempt to > open the ADC device (since it has no idea if it was already opened) but the > second attempt to open the device will result in an error code returned. > Depending on how the code is written in the package, this could be a problem. > Given that an ADC is almost always a mutli-channel peripheral (one adc device > has multple channels) I would suspect the above case to be common: multiple > packages wanting an ADC channel from a single device. > > I am not sure if anything needs to be done here; just wanted to see if folks > thought there should different behavior with regards to the function > returning an error if the device was already opened. If not, folks are going > to have to be careful when they write code using the adc device. Seems to me > if nothing is going to change we have two options: > > 1) The device gets created and opened in some place and handed to the > packages that need it. > 2) The device gets created (say by the bsp) and each package can attempt to > open the device. If os_dev_lookup() returns !NULL but os_dev_open() returns > NULL it means that the device has already been opened. > > Something about #2 just sort of bothers me. I do not like ambiguous stuff > like that; how do you know if there was an error for another reason? Why not: 3) Make the ADC driver consistent with other drivers by adding a reference count. ? I know something less than nothing about the ADC code, so I could certainly be missing something. Chris