Hello Markus, Well, the API is not broken if it does not do anything wrong.
> On Jan 17, 2018, at 12:48 PM, markus <[email protected]> wrote: > > Sorry about the late response - I fell ill. > > 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. > > Looking at the current implementations (11): > - 2 use the HW toggle and just return 0 The 2 that use the toggle and return a 0 probably need to get fixed. You can submit a PR to do so. > - 7 perform a read-write cycle, none is atomic or interrupt safe > - 2 perform a write-read cycle, also not atomic or interrupt safe The hal gives no guarantees of being atomic or interrupt safe. > > The only apps (in the repo) using the return value are slinky and it's > copy&paste sisters, which isn't required because they are reading the pin > beforehand anyway. > Slinky is just an example and should not be used as a defining factor for APIs. Certainly, it can show examples and help the apis expose functionality but it is certainly cannot be used for deciding what to remove. Similar would be the case for any app for that matter. > hal_gpio_toggle as it is adds no value to API and might as well be dropped > entirely. I do not agree. > > > > On Sun, 14 Jan 2018 18:11:56 -0800 > will sanfilippo <[email protected]> wrote: > >> Personally I am not sure of the “official” way to get an API changed >> but possibly another way to go about this is to add a new API that >> does what you describe. This way existing functionality is not >> changed. >> >>> On Jan 12, 2018, at 11:29 AM, markus <[email protected]> wrote: >>> >>> I've been looking at hal_gpio_toggle and it's declaration could be >>> improved by removing the return value: >>> >>> void hal_gpio_toggle(int pin); >>> >>> As it currently stands, by requiring a return value of the pin >>> state one cannot use the HW support of most processors to toggle >>> the pin, and it's also not possible to accomplish in an atomic >>> manner. >>> >>> If the underlying HW support for toggling a pin cannot be used it >>> makes the API call redundant, it can be accomplished by a >>> hal_gpio_read/hal_gpio_write combo. >>> >>> Is there a chance the API can be changed? And how do we go about >>> that? >>> >>> Markus >> > Regards, Vipul Rahane
