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

Rename SoM platform names [ch32184] #1774

Merged
merged 1 commit into from May 13, 2019

Conversation

@technobly
Copy link
Member

commented May 13, 2019

Problem

The Device OS PLATFORM_NAME definition for platforms 22, 23, 24 started as argon-som, boron-som and xenon-som in 1.1.0-rc.1 and 1.1.0-rc.2. The official platform names for the SoMs are A SoM, B SoM and X SoM so in Device OS the PLATFORM_NAME should be renamed to asom, bsom, xsom for compilation.

Solution

  • Update build scripts to reflect the name changes
  • Update USB device descriptor info
  • Update comments and PLATFORM_ARGON_SOM throughout the code to PLATFORM_ASOM, etc..
  • Update previous 1.1.0-rc.x release notes.

Steps to Test

  • If this build compiles, that will test Travis CI
  • Local compilation should also pass:
time ./make_release.sh --platform=asom --tests --debug --publish=1.1.0-rc.2
time ./make_release.sh --platform=bsom --tests --debug  --publish=1.1.0-rc.2
time ./make_release.sh --platform=xsom --tests --debug  --publish=1.1.0-rc.2

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • N/A Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

  • [internal] Rename SoM platform names [ch32184] #1774

@technobly technobly added this to the 1.1.0 milestone May 13, 2019

@technobly technobly requested a review from m-mcgowan May 13, 2019

@m-mcgowan
Copy link
Contributor

left a comment

Looks good. How about adding a step to the unit test bash script that greps the codebase for "PLATFORM_XENON_SOM" and "Xenon SoM" and similarly for A and B. and fails if they are found?

@technobly

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Looks good. How about adding a step to the unit test bash script that greps the codebase for "PLATFORM_XENON_SOM" and "Xenon SoM" and similarly for A and B. and fails if they are found?

I want to say that will unnecessarily slow down the CI job, since there are so many files to search though. Also I don't think it's necessary to be that strict on comments that we'd fail the build for it. Also if anyone tries to use PLATFORM_ARGON_SOM, PLATFORM_BORON_SOM, PLATFORM_XENON_SOM in code out of habit it will fail that it's not defined.

@technobly technobly merged commit dbfb56e into develop May 13, 2019

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 deleted the ch32184/rename-som branch May 13, 2019

@@ -333,13 +333,13 @@ PLATFORM_LWIP=posix
endif

ifeq ("$(PLATFORM_ID)","22")
PLATFORM=argon-som
PLATFORM=asom
PLATFORM_NAME=argon

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

This is used in https://github.com/particle-iot/device-os/blob/develop/hal/src/nRF52840/device_code.cpp#L93, so despite all the changes the broadcast BLE name will still remain (Argon-XXXXXX/Boron-XXXXXX/Xenon-XXXXXX for SoMs).

This comment has been minimized.

Copy link
@technobly

technobly May 14, 2019

Author Member

Thank @avtolstoy :) @monkbroc do we want to broadcast a different name than Argon-XXXXXX, etc.. for SoMs? ASoM-XXXXXX seems a little weird without the space, and I think the space would break things.

This comment has been minimized.

Copy link
@monkbroc

monkbroc May 14, 2019

Member

That's fine. Leave it as is for now as it would break mobile apps otherwise. We can change it later if needed.

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan May 14, 2019

Contributor

We are going to be reworking the mobile apps for the upcoming BLE APIs and changes to how setup mode functions, so there is scope there to rework the apps when we decide on the final name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.