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 >>>> >>> >
