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
