-
Notifications
You must be signed in to change notification settings - Fork 34
Suggested fixes #183
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
Suggested fixes #183
Conversation
5c6555c to
6196faa
Compare
2b61e9c to
7ae62ae
Compare
731e657 to
334b7aa
Compare
cpp/arduino/Arduino.h
Outdated
| */ | ||
|
|
||
| // hardware mocks | ||
| volatile long long __ARDUINO_CI_SFR_MOCK[1024]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that if you defined a variable in a header file then it gets defined every time that header file is included and you get multiple definitions. Instead, if you define it as extern in the header and then put it in a .cpp file, it will be created only once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think this should declare a uin8_t* array, rather than a long long array. If not, I think that each byte in the original "address space" (i.e. offsets passed to _SFR_IOx macros) end up in different long long entries in this array, so multi-byte values do not map properly anymore (i.e. ADC maps to either ADCH or ADCL (dunno which off-hand), rather than being a combination of both).
Also, the current code violates strict type aliasing (accessing a longlong[] through a pointer of a different type is not allowed), which means the compiler might mess up optimizations regarding this array. I was hoping that making it a uint8_t array would solve this, but it seems this only works the other way around (accessing an arbitrary type through a uint8_t*). So maybe this needs access through a union to make it explicit to the compiler what's happening...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a look at the io.h stuff, that (the last commit, the first one that "fixes" _SFR_IO8 only makes it broken in a different way) looks pretty much like what I had in mind for the first part of #193 (comment)
cpp/arduino/Arduino.h
Outdated
| #define _SFR_IO16(io_addr) (*(volatile uint16_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file | ||
| #define _SFR_MEM8(io_addr) (*(volatile uint8_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file | ||
| #define _SFR_MEM16(io_addr) (*(volatile uint16_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file | ||
| #define _SFR_MEM32(io_addr) (*(volatile uint32_t *)(__ARDUINO_CI_SFR_MOCK + io_addr)) // this macro is all we need from the sfr file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave this stuff in io.h, which is where they are used and what any other code that (god knows why) uses them would expect them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking at this and right now the issue is that SampleProjects/TestSomething/test/godmode.cpp fails the spi tests due to bytes being in the wrong order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean here? If you move this to io.h, that makes the tests fail? But not because of include order or something like that, but "byte order"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for completeness, this conversation is continuing in #201.
|
Reverting 8180efb as per this #196 (comment) |
8180efb to
9192004
Compare
The command line option --skip-compilation can be confusing, because unit tests must be compiled before they can run. This adds some text to the flag such that the fact that examples are compiled is acknowledged.
9192004 to
cdf136f
Compare
cdf136f to
f981320
Compare
12d4ae7 to
b0efa36
Compare
643caab to
53437a9
Compare
53437a9 to
cbccf6a
Compare
25ae225 to
ebfa6a7
Compare
Trying it out
You can test this beta of the gem directly from github by putting this in your
Gemfile:Highlights from
CHANGELOG.mdAdded
arduino_ci_remote.rbCLI switch--skip-examples-compilationCppLibrary.header_filesto find header filesLibraryPropertiesto read metadata from Arduino librariesCppLibrary.library_properties_path,CppLibrary.library_properties?,CppLibrary.library_propertiesto expose library properties of a Cpp libraryCppLibrary.arduino_library_dependenciesto list the dependent libraries specified by the library.properties fileChanged
CppLibraryfunctions returning C++ header or code files now respect the 1.0/1.5 library specificationDeprecated
arduino_ci_remote.rbCLI switch--skip-compilationFixed
ostream& operator<<(nullptr_t)if already defined by AppleCppLibrary.in_tests_dir?no longer produces an error if there is no tests directory_SFR_IO8macro no longer produces errors about rvaluesIssues Fixed
skip-compilationoption #175_SFR_IO8no longer visible after definition moved to a new file #196