Skip to content
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

Merged
merged 1 commit into from Nov 28, 2016

Conversation

@sergeuz
Copy link
Member

commented Nov 27, 2016

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

   text	   data	    bss	    dec	    hex	filename
  13724	    304	   1460	  15488	   3c80	../../../build/target/system-part1/platform-10-m/system-part1.elf
   text	   data	    bss	    dec	    hex	filename
 127348	    192	    448	 127988	  1f3f4	../../../build/target/system-part2/platform-10-m/system-part2.elf
   text	   data	    bss	    dec	    hex	filename
 125828	   2496	   3768	 132092	  203fc	../../../build/target/system-part3/platform-10-m/system-part3.elf

After:

   text	   data	    bss	    dec	    hex	filename
  37260	    320	   2140	  39720	   9b28	../../../build/target/system-part1/platform-10-m/system-part1.elf
   text	   data	    bss	    dec	    hex	filename
 127348	    192	    448	 127988	  1f3f4	../../../build/target/system-part2/platform-10-m/system-part2.elf
   text	   data	    bss	    dec	    hex	filename
 107108	   2496	   3272	 112876	  1b8ec	../../../build/target/system-part3/platform-10-m/system-part3.elf

P.S. Don't get confused by the file names :)


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation (n/a)
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Internal

  • [Electron] moved cellular HAL and its direct dependencies from module 2 to module 3 to free up space (this is system-part3 was reduced in size, while system-part1 was increased) [PR #1184]

@sergeuz sergeuz added this to the 0.7.x milestone Nov 27, 2016

@@ -7,6 +8,36 @@

static CellularCredentials cellularCredentials;

static HAL_NET_Callbacks netCallbacks = { 0 };

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Nov 28, 2016

Contributor

I wonder if the callbacks should take a reserved parameter so we can add more info later?

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Nov 28, 2016

Contributor

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.

This comment has been minimized.

Copy link
@technobly

technobly Nov 28, 2016

Member

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.

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Nov 28, 2016

Contributor

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Nov 28, 2016

Author Member

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Nov 28, 2016

Author Member

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.

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Nov 28, 2016

Contributor

you got it! 👍

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2016

I wonder why we gain 25k in one part, but lose only 18k in another. Is there duplication happening?

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2016

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.

@m-mcgowan m-mcgowan merged commit 5113469 into develop Nov 28, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly modified the milestones: 0.7.x, 0.6.1 Nov 29, 2016

@avtolstoy avtolstoy referenced this pull request May 27, 2017
3 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.