-
Notifications
You must be signed in to change notification settings - Fork 513
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
Move cellular HAL to system-part3 #1184
Conversation
@@ -7,6 +8,36 @@ | |||
|
|||
static CellularCredentials cellularCredentials; | |||
|
|||
static HAL_NET_Callbacks netCallbacks = { 0 }; | |||
|
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 wonder if the callbacks should take a reserved parameter so we can add more info later?
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.
On reflection, I see that's not needed - the only eventuality is that the caller (Celluar HAL) passes more data than the callee is expecting to the callback. This is fine with the C calling convention.
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.
So in the future if we need to add to this, we just need to make sure to add extra data to the end of the struct HAL_NET_Callbacks to ensure backwards compatibility.
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.
If we need new callbacks yes...but if we need to extend the existing callbacks with additional parameters, we can just add them to the callback. Older system modules won't see the extra parameters, so that has to be kept in mind, but since they are informational/status that's not a concern - they only mean something if the receiver can understand 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.
hm, does that mean that it's not necessary to add our typical void* reserved
to newly added dynalib functions? That trick when a regular function is called with more arguments than it expects isn't formally allowed in C as far as I know, though it seems to be supported by all major compilers.
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.
disregard the above question. While it's okay to pass more arguments than necessary to a function, it's certainly bad idea to pass fewer arguments, which is the case for dynalib functions without void* reserved
.
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.
you got it! 👍
I wonder why we gain 25k in one part, but lose only 18k in another. Is there duplication happening? |
I think that's something from libc that now happens to be in both part2 and part3. I'll check if there are any suspicious symbols though. |
This PR moves cellular HAL and its direct dependencies from part2 to part3 on Electron. On my system (gcc 5.3.1) this allows to reduce part2 by ~18K:
Before applying this patch (with debugging enabled):
After:
P.S. Don't get confused by the file names :)
Doneness:
Internal