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