You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mynewt.apache.org by markus <ma...@bibi.ca> on 2018/01/12 19:29:59 UTC
HAL API change request
I've been looking at hal_gpio_toggle and it's declaration could be improved by removing the return value:
void hal_gpio_toggle(int pin);
As it currently stands, by requiring a return value of the pin state one cannot use the HW support of most processors to toggle the pin, and it's also not possible to accomplish in an atomic manner.
If the underlying HW support for toggling a pin cannot be used it makes the API call redundant, it can be accomplished by a hal_gpio_read/hal_gpio_write combo.
Is there a chance the API can be changed? And how do we go about that?
Markus
Re: HAL API change request
Posted by will sanfilippo <wi...@runtime.io>.
Either sounds fine to me
> On Jan 17, 2018, at 4:34 PM, markus <ma...@bibi.ca> wrote:
>
> doesn't that imply a guarantee the HAL apparently doesn't make?
>
> how about
> hal_gpio_toggle_basic()
> hal_gpio_toggle_hw()
>
>
> On Wed, 17 Jan 2018 16:23:51 -0800
> will sanfilippo <wi...@runtime.io> wrote:
>
>> hal_gpio_toggle_atomic()
>>
>>
>>> On Jan 17, 2018, at 2:50 PM, markus <ma...@bibi.ca> wrote:
>>>
>>> any suggestions for a name of the new api?
>>>
>>>
>>> On Wed, 17 Jan 2018 14:41:08 -0800
>>> Christopher Collins <ch...@runtime.io> wrote:
>>>
>>>> On Wed, Jan 17, 2018 at 02:33:09PM -0800, markus wrote:
>>>>> I do think there is a write and wrong about toggling a pin. The
>>>>> reason processors and HALs have an API for toggling a pin is to
>>>>> make sure that the pin actually gets toggled and do so in the
>>>>> least amount of processor cycles (precisely because it's such a
>>>>> common operation). Requiring a return value makes usage of such
>>>>> support impossible.
>>>>>
>>>>> Furthermore, an API call in a low level API such as a hal that is
>>>>> (required to be) nothing more than a wrapper around two other API
>>>>> calls of the same API is a waste.
>>>>>
>>>>> ppl make mistakes, APIs evolve, at least the ones that stay around
>>>>> for a while and want to stay relevant. This is a request to make
>>>>> things right and figuring out the process in how to go about
>>>>> it.
>>>>
>>>> The Mynewt policy for breaking backwards compatibility is
>>>> documented here:
>>>> https://cwiki.apache.org/confluence/display/MYNEWT/Release+and+Support+Policy#ReleaseandSupportPolicy-BackwardsCompatibility
>>>>
>>>> So, users should be given plenty of heads up before anything
>>>> changes. I I think Will's idea of adding a new function is a good
>>>> way to start the process.
>>>>
>>>> Chris
>>>>
>>>>>
>>>>>
>>>>> On Wed, 17 Jan 2018 14:10:24 -0800
>>>>> will sanfilippo <wi...@runtime.io> wrote:
>>>>>
>>>>>> Markus (and all):
>>>>>>
>>>>>> I think there may be some confusion here or misinterpretations.
>>>>>>
>>>>>> I think most would agree that there is no absolute requirement
>>>>>> for a hal_gpio_toggle() API if you can both read and write a
>>>>>> gpio. The API was added as a helper since toggling a gpio is a
>>>>>> fairly common construct.
>>>>>>
>>>>>> Regarding its being atomic, the HAL API was not necessarily
>>>>>> designed to be atomic. While some implementations of various HAL
>>>>>> API might be atomic, there is no guarantee that they are nor was
>>>>>> there a requirement that there be. This seems reasonable as
>>>>>> making something atomic on processors that do not provide HW
>>>>>> support for it require extra code. Consider a piece of firmware
>>>>>> where only one task toggles a particular GPIO. Making it atomic
>>>>>> by default would not be necessary.
>>>>>>
>>>>>> Addressing the API itself, whether it is void or returns a value,
>>>>>> is neither right nor wrong in my opinion. Simply because it is
>>>>>> used in a certain way in the repo does not mean that others are
>>>>>> not using it. If it is changed it would break existing code which
>>>>>> is why, in general, API’s are not changed frequently.
>>>>>>
>>>>>> It is for the above reasons that I suggested adding a new API.
>>>>>> This way, you have what you want and there is no breaking of
>>>>>> existing functionality.
>>>>>>
>>>>>> BTW: the fact that certain implementations do not return the
>>>>>> proper value or are incorrect is definitely an issue. These
>>>>>> should be addressed in a PR and appropriate modifications made.
>>>>>>
>>>>>>
>>>>>>> On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
>>>>>>>
>>>>>>> Hi Vipul,
>>>>>>>
>>>>>>>>> The point was to fix a broken API, not to do my own thing ;).
>>>>>>>>>
>>>>>>>>> Looking at the history hal_gpio_toggle was correctly declared
>>>>>>>>> initially and then changed with commit 274da3263 (no rational
>>>>>>>>> given as to why).
>>>>>>>>
>>>>>>>> The change request was sent on the mailing list and there were
>>>>>>>> no issues at the time from the community. If you take a look at
>>>>>>>> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads
>>>>>>>> the gpio anyways because it needs to know the state of the
>>>>>>>> gpio before it toggles it. This read anyways goes waste. If it
>>>>>>>> is returned, it can be used by the calling code. If you want
>>>>>>>> to use the read value, you can do so. If you do not want to
>>>>>>>> use it. You can just ignore the return value.
>>>>>>>
>>>>>>> I don't think that crappy implementations of an API are a
>>>>>>> justification to change the API, they should be a reason to
>>>>>>> change the implementation.
>>>>>>>>> hal_gpio_toggle as it is adds no value to API and might as
>>>>>>>>> well be dropped entirely.
>>>>>>>>
>>>>>>>> I do not agree.
>>>>>>>
>>>>>>> I've stated my reasons why I think the current declaration is
>>>>>>> wrong and redundant - could you please explain why you think
>>>>>>> that's not the case?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Markus
>>>>>>
>>>>>
>>>
>>
>
Re: HAL API change request
Posted by markus <ma...@bibi.ca>.
doesn't that imply a guarantee the HAL apparently doesn't make?
how about
hal_gpio_toggle_basic()
hal_gpio_toggle_hw()
On Wed, 17 Jan 2018 16:23:51 -0800
will sanfilippo <wi...@runtime.io> wrote:
> hal_gpio_toggle_atomic()
>
>
> > On Jan 17, 2018, at 2:50 PM, markus <ma...@bibi.ca> wrote:
> >
> > any suggestions for a name of the new api?
> >
> >
> > On Wed, 17 Jan 2018 14:41:08 -0800
> > Christopher Collins <ch...@runtime.io> wrote:
> >
> >> On Wed, Jan 17, 2018 at 02:33:09PM -0800, markus wrote:
> >>> I do think there is a write and wrong about toggling a pin. The
> >>> reason processors and HALs have an API for toggling a pin is to
> >>> make sure that the pin actually gets toggled and do so in the
> >>> least amount of processor cycles (precisely because it's such a
> >>> common operation). Requiring a return value makes usage of such
> >>> support impossible.
> >>>
> >>> Furthermore, an API call in a low level API such as a hal that is
> >>> (required to be) nothing more than a wrapper around two other API
> >>> calls of the same API is a waste.
> >>>
> >>> ppl make mistakes, APIs evolve, at least the ones that stay around
> >>> for a while and want to stay relevant. This is a request to make
> >>> things right and figuring out the process in how to go about
> >>> it.
> >>
> >> The Mynewt policy for breaking backwards compatibility is
> >> documented here:
> >> https://cwiki.apache.org/confluence/display/MYNEWT/Release+and+Support+Policy#ReleaseandSupportPolicy-BackwardsCompatibility
> >>
> >> So, users should be given plenty of heads up before anything
> >> changes. I I think Will's idea of adding a new function is a good
> >> way to start the process.
> >>
> >> Chris
> >>
> >>>
> >>>
> >>> On Wed, 17 Jan 2018 14:10:24 -0800
> >>> will sanfilippo <wi...@runtime.io> wrote:
> >>>
> >>>> Markus (and all):
> >>>>
> >>>> I think there may be some confusion here or misinterpretations.
> >>>>
> >>>> I think most would agree that there is no absolute requirement
> >>>> for a hal_gpio_toggle() API if you can both read and write a
> >>>> gpio. The API was added as a helper since toggling a gpio is a
> >>>> fairly common construct.
> >>>>
> >>>> Regarding its being atomic, the HAL API was not necessarily
> >>>> designed to be atomic. While some implementations of various HAL
> >>>> API might be atomic, there is no guarantee that they are nor was
> >>>> there a requirement that there be. This seems reasonable as
> >>>> making something atomic on processors that do not provide HW
> >>>> support for it require extra code. Consider a piece of firmware
> >>>> where only one task toggles a particular GPIO. Making it atomic
> >>>> by default would not be necessary.
> >>>>
> >>>> Addressing the API itself, whether it is void or returns a value,
> >>>> is neither right nor wrong in my opinion. Simply because it is
> >>>> used in a certain way in the repo does not mean that others are
> >>>> not using it. If it is changed it would break existing code which
> >>>> is why, in general, API’s are not changed frequently.
> >>>>
> >>>> It is for the above reasons that I suggested adding a new API.
> >>>> This way, you have what you want and there is no breaking of
> >>>> existing functionality.
> >>>>
> >>>> BTW: the fact that certain implementations do not return the
> >>>> proper value or are incorrect is definitely an issue. These
> >>>> should be addressed in a PR and appropriate modifications made.
> >>>>
> >>>>
> >>>>> On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
> >>>>>
> >>>>> Hi Vipul,
> >>>>>
> >>>>>>> The point was to fix a broken API, not to do my own thing ;).
> >>>>>>>
> >>>>>>> Looking at the history hal_gpio_toggle was correctly declared
> >>>>>>> initially and then changed with commit 274da3263 (no rational
> >>>>>>> given as to why).
> >>>>>>
> >>>>>> The change request was sent on the mailing list and there were
> >>>>>> no issues at the time from the community. If you take a look at
> >>>>>> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads
> >>>>>> the gpio anyways because it needs to know the state of the
> >>>>>> gpio before it toggles it. This read anyways goes waste. If it
> >>>>>> is returned, it can be used by the calling code. If you want
> >>>>>> to use the read value, you can do so. If you do not want to
> >>>>>> use it. You can just ignore the return value.
> >>>>>
> >>>>> I don't think that crappy implementations of an API are a
> >>>>> justification to change the API, they should be a reason to
> >>>>> change the implementation.
> >>>>>>> hal_gpio_toggle as it is adds no value to API and might as
> >>>>>>> well be dropped entirely.
> >>>>>>
> >>>>>> I do not agree.
> >>>>>
> >>>>> I've stated my reasons why I think the current declaration is
> >>>>> wrong and redundant - could you please explain why you think
> >>>>> that's not the case?
> >>>>>
> >>>>> Thanks,
> >>>>> Markus
> >>>>
> >>>
> >
>
Re: HAL API change request
Posted by will sanfilippo <wi...@runtime.io>.
hal_gpio_toggle_atomic()
> On Jan 17, 2018, at 2:50 PM, markus <ma...@bibi.ca> wrote:
>
> any suggestions for a name of the new api?
>
>
> On Wed, 17 Jan 2018 14:41:08 -0800
> Christopher Collins <ch...@runtime.io> wrote:
>
>> On Wed, Jan 17, 2018 at 02:33:09PM -0800, markus wrote:
>>> I do think there is a write and wrong about toggling a pin. The
>>> reason processors and HALs have an API for toggling a pin is to
>>> make sure that the pin actually gets toggled and do so in the least
>>> amount of processor cycles (precisely because it's such a common
>>> operation). Requiring a return value makes usage of such support
>>> impossible.
>>>
>>> Furthermore, an API call in a low level API such as a hal that is
>>> (required to be) nothing more than a wrapper around two other API
>>> calls of the same API is a waste.
>>>
>>> ppl make mistakes, APIs evolve, at least the ones that stay around
>>> for a while and want to stay relevant. This is a request to make
>>> things right and figuring out the process in how to go about it.
>>
>> The Mynewt policy for breaking backwards compatibility is documented
>> here:
>> https://cwiki.apache.org/confluence/display/MYNEWT/Release+and+Support+Policy#ReleaseandSupportPolicy-BackwardsCompatibility
>>
>> So, users should be given plenty of heads up before anything
>> changes. I I think Will's idea of adding a new function is a good
>> way to start the process.
>>
>> Chris
>>
>>>
>>>
>>> On Wed, 17 Jan 2018 14:10:24 -0800
>>> will sanfilippo <wi...@runtime.io> wrote:
>>>
>>>> Markus (and all):
>>>>
>>>> I think there may be some confusion here or misinterpretations.
>>>>
>>>> I think most would agree that there is no absolute requirement
>>>> for a hal_gpio_toggle() API if you can both read and write a
>>>> gpio. The API was added as a helper since toggling a gpio is a
>>>> fairly common construct.
>>>>
>>>> Regarding its being atomic, the HAL API was not necessarily
>>>> designed to be atomic. While some implementations of various HAL
>>>> API might be atomic, there is no guarantee that they are nor was
>>>> there a requirement that there be. This seems reasonable as
>>>> making something atomic on processors that do not provide HW
>>>> support for it require extra code. Consider a piece of firmware
>>>> where only one task toggles a particular GPIO. Making it atomic
>>>> by default would not be necessary.
>>>>
>>>> Addressing the API itself, whether it is void or returns a value,
>>>> is neither right nor wrong in my opinion. Simply because it is
>>>> used in a certain way in the repo does not mean that others are
>>>> not using it. If it is changed it would break existing code which
>>>> is why, in general, API’s are not changed frequently.
>>>>
>>>> It is for the above reasons that I suggested adding a new API.
>>>> This way, you have what you want and there is no breaking of
>>>> existing functionality.
>>>>
>>>> BTW: the fact that certain implementations do not return the
>>>> proper value or are incorrect is definitely an issue. These
>>>> should be addressed in a PR and appropriate modifications made.
>>>>
>>>>
>>>>> On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
>>>>>
>>>>> Hi Vipul,
>>>>>
>>>>>>> The point was to fix a broken API, not to do my own thing ;).
>>>>>>>
>>>>>>> Looking at the history hal_gpio_toggle was correctly declared
>>>>>>> initially and then changed with commit 274da3263 (no rational
>>>>>>> given as to why).
>>>>>>
>>>>>> The change request was sent on the mailing list and there were
>>>>>> no issues at the time from the community. If you take a look at
>>>>>> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads
>>>>>> the gpio anyways because it needs to know the state of the
>>>>>> gpio before it toggles it. This read anyways goes waste. If it
>>>>>> is returned, it can be used by the calling code. If you want
>>>>>> to use the read value, you can do so. If you do not want to
>>>>>> use it. You can just ignore the return value.
>>>>>
>>>>> I don't think that crappy implementations of an API are a
>>>>> justification to change the API, they should be a reason to
>>>>> change the implementation.
>>>>>>> hal_gpio_toggle as it is adds no value to API and might as
>>>>>>> well be dropped entirely.
>>>>>>
>>>>>> I do not agree.
>>>>>
>>>>> I've stated my reasons why I think the current declaration is
>>>>> wrong and redundant - could you please explain why you think
>>>>> that's not the case?
>>>>>
>>>>> Thanks,
>>>>> Markus
>>>>
>>>
>
Re: HAL API change request
Posted by markus <ma...@bibi.ca>.
any suggestions for a name of the new api?
On Wed, 17 Jan 2018 14:41:08 -0800
Christopher Collins <ch...@runtime.io> wrote:
> On Wed, Jan 17, 2018 at 02:33:09PM -0800, markus wrote:
> > I do think there is a write and wrong about toggling a pin. The
> > reason processors and HALs have an API for toggling a pin is to
> > make sure that the pin actually gets toggled and do so in the least
> > amount of processor cycles (precisely because it's such a common
> > operation). Requiring a return value makes usage of such support
> > impossible.
> >
> > Furthermore, an API call in a low level API such as a hal that is
> > (required to be) nothing more than a wrapper around two other API
> > calls of the same API is a waste.
> >
> > ppl make mistakes, APIs evolve, at least the ones that stay around
> > for a while and want to stay relevant. This is a request to make
> > things right and figuring out the process in how to go about it.
>
> The Mynewt policy for breaking backwards compatibility is documented
> here:
> https://cwiki.apache.org/confluence/display/MYNEWT/Release+and+Support+Policy#ReleaseandSupportPolicy-BackwardsCompatibility
>
> So, users should be given plenty of heads up before anything
> changes. I I think Will's idea of adding a new function is a good
> way to start the process.
>
> Chris
>
> >
> >
> > On Wed, 17 Jan 2018 14:10:24 -0800
> > will sanfilippo <wi...@runtime.io> wrote:
> >
> > > Markus (and all):
> > >
> > > I think there may be some confusion here or misinterpretations.
> > >
> > > I think most would agree that there is no absolute requirement
> > > for a hal_gpio_toggle() API if you can both read and write a
> > > gpio. The API was added as a helper since toggling a gpio is a
> > > fairly common construct.
> > >
> > > Regarding its being atomic, the HAL API was not necessarily
> > > designed to be atomic. While some implementations of various HAL
> > > API might be atomic, there is no guarantee that they are nor was
> > > there a requirement that there be. This seems reasonable as
> > > making something atomic on processors that do not provide HW
> > > support for it require extra code. Consider a piece of firmware
> > > where only one task toggles a particular GPIO. Making it atomic
> > > by default would not be necessary.
> > >
> > > Addressing the API itself, whether it is void or returns a value,
> > > is neither right nor wrong in my opinion. Simply because it is
> > > used in a certain way in the repo does not mean that others are
> > > not using it. If it is changed it would break existing code which
> > > is why, in general, API’s are not changed frequently.
> > >
> > > It is for the above reasons that I suggested adding a new API.
> > > This way, you have what you want and there is no breaking of
> > > existing functionality.
> > >
> > > BTW: the fact that certain implementations do not return the
> > > proper value or are incorrect is definitely an issue. These
> > > should be addressed in a PR and appropriate modifications made.
> > >
> > >
> > > > On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
> > > >
> > > > Hi Vipul,
> > > >
> > > >>> The point was to fix a broken API, not to do my own thing ;).
> > > >>>
> > > >>> Looking at the history hal_gpio_toggle was correctly declared
> > > >>> initially and then changed with commit 274da3263 (no rational
> > > >>> given as to why).
> > > >>
> > > >> The change request was sent on the mailing list and there were
> > > >> no issues at the time from the community. If you take a look at
> > > >> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads
> > > >> the gpio anyways because it needs to know the state of the
> > > >> gpio before it toggles it. This read anyways goes waste. If it
> > > >> is returned, it can be used by the calling code. If you want
> > > >> to use the read value, you can do so. If you do not want to
> > > >> use it. You can just ignore the return value.
> > > >
> > > > I don't think that crappy implementations of an API are a
> > > > justification to change the API, they should be a reason to
> > > > change the implementation.
> > > >>> hal_gpio_toggle as it is adds no value to API and might as
> > > >>> well be dropped entirely.
> > > >>
> > > >> I do not agree.
> > > >
> > > > I've stated my reasons why I think the current declaration is
> > > > wrong and redundant - could you please explain why you think
> > > > that's not the case?
> > > >
> > > > Thanks,
> > > > Markus
> > >
> >
Re: HAL API change request
Posted by Christopher Collins <ch...@runtime.io>.
On Wed, Jan 17, 2018 at 02:33:09PM -0800, markus wrote:
> I do think there is a write and wrong about toggling a pin. The reason processors and HALs have an API for toggling a pin is to make sure that the pin actually gets toggled and do so in the least amount of processor cycles (precisely because it's such a common operation). Requiring a return value makes usage of such support impossible.
>
> Furthermore, an API call in a low level API such as a hal that is (required to be) nothing more than a wrapper around two other API calls of the same API is a waste.
>
> ppl make mistakes, APIs evolve, at least the ones that stay around for a while and want to stay relevant. This is a request to make things right and figuring out the process in how to go about it.
The Mynewt policy for breaking backwards compatibility is documented
here:
https://cwiki.apache.org/confluence/display/MYNEWT/Release+and+Support+Policy#ReleaseandSupportPolicy-BackwardsCompatibility
So, users should be given plenty of heads up before anything changes. I
I think Will's idea of adding a new function is a good way to start the
process.
Chris
>
>
> On Wed, 17 Jan 2018 14:10:24 -0800
> will sanfilippo <wi...@runtime.io> wrote:
>
> > Markus (and all):
> >
> > I think there may be some confusion here or misinterpretations.
> >
> > I think most would agree that there is no absolute requirement for a
> > hal_gpio_toggle() API if you can both read and write a gpio. The API
> > was added as a helper since toggling a gpio is a fairly common
> > construct.
> >
> > Regarding its being atomic, the HAL API was not necessarily designed
> > to be atomic. While some implementations of various HAL API might be
> > atomic, there is no guarantee that they are nor was there a
> > requirement that there be. This seems reasonable as making something
> > atomic on processors that do not provide HW support for it require
> > extra code. Consider a piece of firmware where only one task toggles
> > a particular GPIO. Making it atomic by default would not be necessary.
> >
> > Addressing the API itself, whether it is void or returns a value, is
> > neither right nor wrong in my opinion. Simply because it is used in a
> > certain way in the repo does not mean that others are not using it.
> > If it is changed it would break existing code which is why, in
> > general, API’s are not changed frequently.
> >
> > It is for the above reasons that I suggested adding a new API. This
> > way, you have what you want and there is no breaking of existing
> > functionality.
> >
> > BTW: the fact that certain implementations do not return the proper
> > value or are incorrect is definitely an issue. These should be
> > addressed in a PR and appropriate modifications made.
> >
> >
> > > On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
> > >
> > > Hi Vipul,
> > >
> > >>> The point was to fix a broken API, not to do my own thing ;).
> > >>>
> > >>> Looking at the history hal_gpio_toggle was correctly declared
> > >>> initially and then changed with commit 274da3263 (no rational
> > >>> given as to why).
> > >>
> > >> The change request was sent on the mailing list and there were no
> > >> issues at the time from the community. If you take a look at
> > >> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads the
> > >> gpio anyways because it needs to know the state of the gpio before
> > >> it toggles it. This read anyways goes waste. If it is returned, it
> > >> can be used by the calling code. If you want to use the read
> > >> value, you can do so. If you do not want to use it. You can just
> > >> ignore the return value.
> > >
> > > I don't think that crappy implementations of an API are a
> > > justification to change the API, they should be a reason to change
> > > the implementation.
> > >>> hal_gpio_toggle as it is adds no value to API and might as well be
> > >>> dropped entirely.
> > >>
> > >> I do not agree.
> > >
> > > I've stated my reasons why I think the current declaration is wrong
> > > and redundant - could you please explain why you think that's not
> > > the case?
> > >
> > > Thanks,
> > > Markus
> >
>
Re: HAL API change request
Posted by markus <ma...@bibi.ca>.
I do think there is a write and wrong about toggling a pin. The reason processors and HALs have an API for toggling a pin is to make sure that the pin actually gets toggled and do so in the least amount of processor cycles (precisely because it's such a common operation). Requiring a return value makes usage of such support impossible.
Furthermore, an API call in a low level API such as a hal that is (required to be) nothing more than a wrapper around two other API calls of the same API is a waste.
ppl make mistakes, APIs evolve, at least the ones that stay around for a while and want to stay relevant. This is a request to make things right and figuring out the process in how to go about it.
On Wed, 17 Jan 2018 14:10:24 -0800
will sanfilippo <wi...@runtime.io> wrote:
> Markus (and all):
>
> I think there may be some confusion here or misinterpretations.
>
> I think most would agree that there is no absolute requirement for a
> hal_gpio_toggle() API if you can both read and write a gpio. The API
> was added as a helper since toggling a gpio is a fairly common
> construct.
>
> Regarding its being atomic, the HAL API was not necessarily designed
> to be atomic. While some implementations of various HAL API might be
> atomic, there is no guarantee that they are nor was there a
> requirement that there be. This seems reasonable as making something
> atomic on processors that do not provide HW support for it require
> extra code. Consider a piece of firmware where only one task toggles
> a particular GPIO. Making it atomic by default would not be necessary.
>
> Addressing the API itself, whether it is void or returns a value, is
> neither right nor wrong in my opinion. Simply because it is used in a
> certain way in the repo does not mean that others are not using it.
> If it is changed it would break existing code which is why, in
> general, API’s are not changed frequently.
>
> It is for the above reasons that I suggested adding a new API. This
> way, you have what you want and there is no breaking of existing
> functionality.
>
> BTW: the fact that certain implementations do not return the proper
> value or are incorrect is definitely an issue. These should be
> addressed in a PR and appropriate modifications made.
>
>
> > On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
> >
> > Hi Vipul,
> >
> >>> The point was to fix a broken API, not to do my own thing ;).
> >>>
> >>> Looking at the history hal_gpio_toggle was correctly declared
> >>> initially and then changed with commit 274da3263 (no rational
> >>> given as to why).
> >>
> >> The change request was sent on the mailing list and there were no
> >> issues at the time from the community. If you take a look at
> >> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads the
> >> gpio anyways because it needs to know the state of the gpio before
> >> it toggles it. This read anyways goes waste. If it is returned, it
> >> can be used by the calling code. If you want to use the read
> >> value, you can do so. If you do not want to use it. You can just
> >> ignore the return value.
> >
> > I don't think that crappy implementations of an API are a
> > justification to change the API, they should be a reason to change
> > the implementation.
> >>> hal_gpio_toggle as it is adds no value to API and might as well be
> >>> dropped entirely.
> >>
> >> I do not agree.
> >
> > I've stated my reasons why I think the current declaration is wrong
> > and redundant - could you please explain why you think that's not
> > the case?
> >
> > Thanks,
> > Markus
>
Re: HAL API change request
Posted by will sanfilippo <wi...@runtime.io>.
Markus (and all):
I think there may be some confusion here or misinterpretations.
I think most would agree that there is no absolute requirement for a hal_gpio_toggle() API if you can both read and write a gpio. The API was added as a helper since toggling a gpio is a fairly common construct.
Regarding its being atomic, the HAL API was not necessarily designed to be atomic. While some implementations of various HAL API might be atomic, there is no guarantee that they are nor was there a requirement that there be. This seems reasonable as making something atomic on processors that do not provide HW support for it require extra code. Consider a piece of firmware where only one task toggles a particular GPIO. Making it atomic by default would not be necessary.
Addressing the API itself, whether it is void or returns a value, is neither right nor wrong in my opinion. Simply because it is used in a certain way in the repo does not mean that others are not using it. If it is changed it would break existing code which is why, in general, API’s are not changed frequently.
It is for the above reasons that I suggested adding a new API. This way, you have what you want and there is no breaking of existing functionality.
BTW: the fact that certain implementations do not return the proper value or are incorrect is definitely an issue. These should be addressed in a PR and appropriate modifications made.
> On Jan 17, 2018, at 1:48 PM, markus <ma...@bibi.ca> wrote:
>
> Hi Vipul,
>
>>> The point was to fix a broken API, not to do my own thing ;).
>>>
>>> Looking at the history hal_gpio_toggle was correctly declared
>>> initially and then changed with commit 274da3263 (no rational given
>>> as to why).
>>
>> The change request was sent on the mailing list and there were no
>> issues at the time from the community. If you take a look at
>> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads the
>> gpio anyways because it needs to know the state of the gpio before it
>> toggles it. This read anyways goes waste. If it is returned, it can
>> be used by the calling code. If you want to use the read value, you
>> can do so. If you do not want to use it. You can just ignore the
>> return value.
>
> I don't think that crappy implementations of an API are a justification to change the API, they should be a reason to change the implementation.
>
>>> hal_gpio_toggle as it is adds no value to API and might as well be
>>> dropped entirely.
>>
>> I do not agree.
>
> I've stated my reasons why I think the current declaration is wrong and redundant - could you please explain why you think that's not the case?
>
> Thanks,
> Markus
Re: HAL API change request
Posted by markus <ma...@bibi.ca>.
Hi Vipul,
> > The point was to fix a broken API, not to do my own thing ;).
> >
> > Looking at the history hal_gpio_toggle was correctly declared
> > initially and then changed with commit 274da3263 (no rational given
> > as to why).
>
> The change request was sent on the mailing list and there were no
> issues at the time from the community. If you take a look at
> hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads the
> gpio anyways because it needs to know the state of the gpio before it
> toggles it. This read anyways goes waste. If it is returned, it can
> be used by the calling code. If you want to use the read value, you
> can do so. If you do not want to use it. You can just ignore the
> return value.
I don't think that crappy implementations of an API are a justification to change the API, they should be a reason to change the implementation.
> > hal_gpio_toggle as it is adds no value to API and might as well be
> > dropped entirely.
>
> I do not agree.
I've stated my reasons why I think the current declaration is wrong and redundant - could you please explain why you think that's not the case?
Thanks,
Markus
Re: HAL API change request
Posted by Vipul Rahane <vi...@runtime.io>.
Hello Markus,
Well, the API is not broken if it does not do anything wrong.
> On Jan 17, 2018, at 12:48 PM, markus <ma...@bibi.ca> wrote:
>
> Sorry about the late response - I fell ill.
>
> The point was to fix a broken API, not to do my own thing ;).
>
> Looking at the history hal_gpio_toggle was correctly declared initially and then changed with commit 274da3263 (no rational given as to why).
The change request was sent on the mailing list and there were no issues at the time from the community.
If you take a look at hw/mcu/nordic/nrf52xxx/src/hal_gpio.c, hal_gpi_toggle() reads the gpio anyways because it needs to know the state of the gpio before it toggles it. This read anyways goes waste. If it is returned, it can be used by the calling code. If you want to use the read value, you can do so. If you do not want to use it. You can just ignore the return value.
>
> Looking at the current implementations (11):
> - 2 use the HW toggle and just return 0
The 2 that use the toggle and return a 0 probably need to get fixed. You can submit a PR to do so.
> - 7 perform a read-write cycle, none is atomic or interrupt safe
> - 2 perform a write-read cycle, also not atomic or interrupt safe
The hal gives no guarantees of being atomic or interrupt safe.
>
> The only apps (in the repo) using the return value are slinky and it's copy&paste sisters, which isn't required because they are reading the pin beforehand anyway.
>
Slinky is just an example and should not be used as a defining factor for APIs. Certainly, it can show examples and help the apis expose functionality but it is certainly cannot be used for deciding what to remove. Similar would be the case for any app for that matter.
> hal_gpio_toggle as it is adds no value to API and might as well be dropped entirely.
I do not agree.
>
>
>
> On Sun, 14 Jan 2018 18:11:56 -0800
> will sanfilippo <wi...@runtime.io> wrote:
>
>> Personally I am not sure of the “official” way to get an API changed
>> but possibly another way to go about this is to add a new API that
>> does what you describe. This way existing functionality is not
>> changed.
>>
>>> On Jan 12, 2018, at 11:29 AM, markus <ma...@bibi.ca> wrote:
>>>
>>> I've been looking at hal_gpio_toggle and it's declaration could be
>>> improved by removing the return value:
>>>
>>> void hal_gpio_toggle(int pin);
>>>
>>> As it currently stands, by requiring a return value of the pin
>>> state one cannot use the HW support of most processors to toggle
>>> the pin, and it's also not possible to accomplish in an atomic
>>> manner.
>>>
>>> If the underlying HW support for toggling a pin cannot be used it
>>> makes the API call redundant, it can be accomplished by a
>>> hal_gpio_read/hal_gpio_write combo.
>>>
>>> Is there a chance the API can be changed? And how do we go about
>>> that?
>>>
>>> Markus
>>
>
Regards,
Vipul Rahane
Re: HAL API change request
Posted by markus <ma...@bibi.ca>.
Sorry about the late response - I fell ill.
The point was to fix a broken API, not to do my own thing ;).
Looking at the history hal_gpio_toggle was correctly declared initially and then changed with commit 274da3263 (no rational given as to why).
Looking at the current implementations (11):
- 2 use the HW toggle and just return 0
- 7 perform a read-write cycle, none is atomic or interrupt safe
- 2 perform a write-read cycle, also not atomic or interrupt safe
The only apps (in the repo) using the return value are slinky and it's copy&paste sisters, which isn't required because they are reading the pin beforehand anyway.
hal_gpio_toggle as it is adds no value to API and might as well be dropped entirely.
On Sun, 14 Jan 2018 18:11:56 -0800
will sanfilippo <wi...@runtime.io> wrote:
> Personally I am not sure of the “official” way to get an API changed
> but possibly another way to go about this is to add a new API that
> does what you describe. This way existing functionality is not
> changed.
>
> > On Jan 12, 2018, at 11:29 AM, markus <ma...@bibi.ca> wrote:
> >
> > I've been looking at hal_gpio_toggle and it's declaration could be
> > improved by removing the return value:
> >
> > void hal_gpio_toggle(int pin);
> >
> > As it currently stands, by requiring a return value of the pin
> > state one cannot use the HW support of most processors to toggle
> > the pin, and it's also not possible to accomplish in an atomic
> > manner.
> >
> > If the underlying HW support for toggling a pin cannot be used it
> > makes the API call redundant, it can be accomplished by a
> > hal_gpio_read/hal_gpio_write combo.
> >
> > Is there a chance the API can be changed? And how do we go about
> > that?
> >
> > Markus
>
Re: HAL API change request
Posted by will sanfilippo <wi...@runtime.io>.
Personally I am not sure of the “official” way to get an API changed but possibly another way to go about this is to add a new API that does what you describe. This way existing functionality is not changed.
> On Jan 12, 2018, at 11:29 AM, markus <ma...@bibi.ca> wrote:
>
> I've been looking at hal_gpio_toggle and it's declaration could be improved by removing the return value:
>
> void hal_gpio_toggle(int pin);
>
> As it currently stands, by requiring a return value of the pin state one cannot use the HW support of most processors to toggle the pin, and it's also not possible to accomplish in an atomic manner.
>
> If the underlying HW support for toggling a pin cannot be used it makes the API call redundant, it can be accomplished by a hal_gpio_read/hal_gpio_write combo.
>
> Is there a chance the API can be changed? And how do we go about that?
>
> Markus