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