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 <[email protected]> wrote: > hal_gpio_toggle_atomic() > > > > On Jan 17, 2018, at 2:50 PM, markus <[email protected]> wrote: > > > > any suggestions for a name of the new api? > > > > > > On Wed, 17 Jan 2018 14:41:08 -0800 > > Christopher Collins <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 > >>>> > >>> > > >
