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.

Sign up for GitHub

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

Jump to bottom

Directory structure adr #15

Merged
merged 6 commits into from
Dec 11, 2020
Merged

Directory structure adr #15

merged 6 commits into from
Dec 11, 2020

Conversation

paul-szczepanek-arm
Copy link
Member

We need to agree on this so we don't clash. Please comment on the proposal.

pan-
pan- reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Paul, what do you think of making the inclusion pattern consistent with Mbed OS ?

doc/adr/0003-directory-structure.md Outdated
An example service called `example` would look like this:

```
services/ble-service-example/include/example.h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not in line with what's in mbed-os at the moment which is include/library-name to include the header files with library-name/header.h.

doc/adr/0003-directory-structure.md Show resolved Hide resolved
doc/adr/0003-directory-structure.md Outdated
@@ -0,0 +1,39 @@
# 3. Define initial testing infrastructure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the name of this ADR.

@pan-
Copy link
Member

pan- commented Dec 1, 2020

You forgot to change the include structure in 003fe26

@paul-szczepanek-arm
Copy link
Member Author

Sorry, it's the constant timeouts yesterday (it messes up github desktop).

@pan-
Copy link
Member

pan- commented Dec 2, 2020

@AGlass0fMilk Are you happy with the proposed structure ?

AGlass0fMilk
AGlass0fMilk requested changes Dec 7, 2020
View reviewed changes
doc/adr/0003-directory-structure.md Outdated Show resolved Hide resolved
doc/adr/0003-directory-structure.md Show resolved Hide resolved
@paul-szczepanek-arm
Copy link
Member Author

I thought the idea was that once the service stops being experimental it can be moved to mbed-os. Is that not the case?

@pan-
Copy link
Member

pan- commented Dec 8, 2020 •
edited
Loading

I thought the idea was that once the service stops being experimental it can be moved to mbed-os. Is that not the case?

It is all about the level of support and integration. Given the trend I think it would not be moved to Mbed OS.

@AGlass0fMilk It would be great to have reusable utilities/helper classes shared across the services of the repo.
I can open an ADR to discuss of the location.

pan-
pan- reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget about my last comment, this is the ADR for the directory structure.

I added a proposal for extensions structure. Let me know what you think.

doc/adr/0003-directory-structure.md Show resolved Hide resolved
doc/adr/0003-directory-structure.md
so it's easy for the user to pick up any one service.

## Decision

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can split the Decision in several parts: services and extensions. Maybe we could mention test that goes into tests. I don't think there is a need to detail it at the moment.

For extensions, what do you think of having them in an extensions folder organized like Mbed OS BLE ?

  • extensions/include/ble/gatt
  • extensions/include/ble/gap
  • extensions/include/ble/common
  • extensions/source/...

That way, include path shouldn't change if these extensions are included into Mbed OS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, might as well put this into this adr

pan-
pan- approved these changes Dec 8, 2020
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @paul-szczepanek-arm it is good to have more details.

@pan-
Copy link
Member

pan- commented Dec 9, 2020

@AGlass0fMilk Are you satisfied with the updates made to the document ?
It would be good to reach consensus rapidly so it doesn't block other PRs.

@pan- pan- merged commit 8231be8 into ARMmbed:main Dec 11, 2020
noonfom pushed a commit to noonfom/mbed-os-experimental-ble-services that referenced this pull request Dec 16, 2020
@paul-szczepanek-arm @AGlass0fMilk
Directory structure adr (ARMmbed#15)
617e7e4 
* directory structure adr

* fix include strcuture and fix name of adr

* fix include structure

* Fix typo in adr

Co-authored-by: George Beckstein <[email protected]>

* include extensions

* add tests info

Co-authored-by: George Beckstein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan- pan- pan- approved these changes

@AGlass0fMilk AGlass0fMilk AGlass0fMilk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paul-szczepanek-arm @pan- @AGlass0fMilk