Skip to content

Refactor volatile operations (try 2) #334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 14, 2019
Merged

Refactor volatile operations (try 2) #334

merged 4 commits into from
May 14, 2019

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented May 8, 2019

This replaces #325 with a better to use API.

I tested examples/blinky1 and examples/blinky2 on an Arduino Uno, and tested examples/blinky1 on a digispark. They all still work, but code size is significantly increased. I'll improve this PR to avoid that.

@aykevl
Copy link
Member Author

aykevl commented May 8, 2019

This refactor is also part of #285. Removing //go: volatile will make that possible.

@deadprogram
Copy link
Member

My first impression is really positive. The code looks a lot clearer and cleaner this way than using all the operators.

@aykevl aykevl force-pushed the volatile-refactor-2 branch from f0f0c82 to 9dca67f Compare May 10, 2019 20:32
@aykevl aykevl marked this pull request as ready for review May 10, 2019 20:32
@aykevl
Copy link
Member Author

aykevl commented May 10, 2019

I fixed it (mostly) with //go:inline hints. I'm much happier with those because 1) they are set on a function definition and not on a declaration (thus are only relevant to the package itself, the information does not have to travel package boundaries) and 2) it is only a hint, it does not require anything from the compiler. In fact, gc will ignore it (see golang/go#21536). The //go:volatile hack is basically an extension of the type system, having all sorts of weird edge cases like #151.

Unfortunately, code size does change. For examples/blinky1 and examples/adc it gets slightly smaller, and for examples/echo it gets a bit bigger. Behavior is unchanged in my tests. I'm slightly worried it might badly affect the other targets but if it does, we can look for alternatives. I prefer this API much more than the current/old compiler hints/hacks so we should make the compiler smart enough to optimize it away.

@deadprogram
Copy link
Member

OK, so seems like we will need the equivilent PR for the various ARM-based MCUs. There are a lot more implementations to port, I am happy to help with that once you have the core code. We should merge any outstanding machine package commits first, if possible.

Also, just wondering if perhaps the AVR SPI implementation that @torntrousers was working on might be ready to merge before this PR lands? Otherwise there will be some changes he will need to make first.

I looked at the code size differences a little, and they were very small, as in 16-64 bytes total. That is easily worth the benefits, IMO.

Anyhow, we should figure out how to proceed, and do it as soon as possible. I really want this API now that I have seen it.

@aykevl
Copy link
Member Author

aykevl commented May 13, 2019

So which one should go in first? I see there is a merge conflict that's trivial to resolve (EDIT: fixed), but other than that I would prefer to merge this one first as long as there is no PR from @torntrousers (unless he intends to make one soon). The longer this one waits, the more maintenance it will require.

aykevl added 4 commits May 13, 2019 17:21
This does increase code size, but it is necessary to avoid undefined
behavior.
The long term goal is to remove the //go:volatile hack.
This avoids the //go:volatile pragma on types in Go source code, at
least for AVR targets.
@aykevl aykevl force-pushed the volatile-refactor-2 branch from 9dca67f to edfbae5 Compare May 13, 2019 15:24
@torntrousers
Copy link
Contributor

Please don't wait for me. Been held up by life and all the i2c things.

@deadprogram
Copy link
Member

Thanks for the update @torntrousers when you do get around to it, the changes should not be very substantial.

OK I will merge this PR so we can keep going.

The only ARM-based PR still outstanding is the one from @Munsio #333 which would appear to be nearly complete. Seems like would be very good to merge that one before starting the ARM migration, probably.

@deadprogram deadprogram merged commit e0cf74e into dev May 14, 2019
@deadprogram deadprogram deleted the volatile-refactor-2 branch May 14, 2019 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants