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