Either sounds fine to me
> On Jan 17, 2018, at 4:34 PM, markus <[email protected]> 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 <[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 >>>>>> >>>>> >>> >> >
