From 52d19065893f6af66fa0e1c426852b820358f3b5 Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Sun, 7 Nov 2021 18:30:29 +0200 Subject: Separate and update coding conventions and contributing pages --- doc/contribute.md | 58 ++++++++----------------------------------------------- 1 file changed, 8 insertions(+), 50 deletions(-) (limited to 'doc/contribute.md') diff --git a/doc/contribute.md b/doc/contribute.md index 595a5996..70fc567c 100644 --- a/doc/contribute.md +++ b/doc/contribute.md @@ -14,10 +14,6 @@ As the documentation is part of the source code, you can submit your improvement You want to fix a bug, add a cool new functionality or improve the code? See *How to submit a pull request below*. -## Spread the word - -The Pinetime is a cool open source project that deserves to be known. Talk about it around you, on social networks, on your blog,... and let people know that we are working on an open source firmware for a smartwatch! - # How to submit a pull request? ## TL;DR @@ -25,7 +21,7 @@ The Pinetime is a cool open source project that deserves to be known. Talk about - Create a branch from develop - Work on a single subject in this branch. Create multiple branches/pulls-requests if you want to work on multiple subjects (bugs, features,...) - Test your modifications on the actual hardware - - Check the code formatting against our coding conventions and [clang-format](../.clang-format) and [clang-tidy](../.clang-tidy) + - Check your code against the [coding conventions](/doc/coding-convention.md) and [clang-format](../.clang-format) and [clang-tidy](../.clang-tidy) - Clean your code and remove files that are not needed - Write documentation related to your new feature if applicable - Create a pull request and write a great description about it: what does your PR do, why, how,... Add pictures and video if possible @@ -38,9 +34,9 @@ If you want to fix a bug, add functionality or improve the code, you'll first ne When your feature branch is ready, **make sure it actually works** and **do not forget to write documentation** about it if it's relevant. -**Creating a pull request containing modifications that haven't been tested is strongly discouraged.** If, for any reason, you cannot test your modifications but want to publish them anyway, **please mention it in the description**. This way, other contributors might be willing to test it and provide feedback about your code. +**Creating a pull request containing modifications that haven't been tested is strongly discouraged.** If for any reason you cannot test your modifications, but want to publish them anyway, **please mention it in the description**. This way, other contributors might be willing to test it and provide feedback about your code. -Also, before submitting your PR, check the coding style of your code against the **coding conventions** detailed below. This project also provides [clang-format](../.clang-format) and [clang-tidy](../.clang-tidy) configuration files. You can use them to ensure correct formatting of your code. +Before submitting a PR, check your code against our [coding conventions](/doc/coding-convention.md). This project also provides [clang-format](../.clang-format) and [clang-tidy](../.clang-tidy) configuration files. You should use them to ensure correct formatting of your code. Don't forget to check the files you are going to commit and remove those which aren't necessary (config files from your IDE, for example). Remove old comments, commented code,... @@ -52,52 +48,14 @@ Once the pull request is reviewed and accepted, it'll be merged into **develop** ## Why all these rules? -Reviewing pull requests is a **very time consuming task** for the creator of this project ([JF002](https://github.com/JF002)) and for other contributors who take the time to review them. Everything you do to make reviewing easier will **get your PR merged faster**. +Reviewing pull requests is a **very time consuming task**. Everything you do to make reviewing easier will **get your PR merged faster**. -When reviewing PRs, the author and contributors will first look at the **description**. If it's easy to understand what the PR does, why the modification is needed or interesting and how it's done, a good part of the work is already done : we understand the PR and its context. +Reviewers will first look at the **description**. If it's easy to understand what the PR does, why the modification is needed or interesting and how it's done, a good part of the work is already done : we understand the PR and its context. -Then, reviewing **a few files that were modified for a single purpose** is a lot more easier than to review 30 files modified for many reasons (bug fix, UI improvements, typos in doc,...), even if all these changes make sense. Also, it's possible that we agree on some modification but not on some other, so we won't be able to merge the PR because of the changes that are not accepted. +Reviewing **a few files that were modified for a single purpose** is a lot easier than reviewing 30 files modified for many reasons (bug fix, UI improvements, typos in doc,...), even if all the changes make sense. Also, it's possible that we agree on some modification but not on another, so we won't be able to merge the PR because of the changes that are not accepted. -We do our best to keep the code as consistent as possible. If the formatting of the code in your PR is not consistent with our code base, we'll ask you to review it, which will take more time. +We do our best to keep the code as consistent as possible. If the formatting of your code is not consistent with our code base, we'll ask you to review it. -The last step of the review consists of **testing** the modification. If it doesn't work out of the box, we'll ask your to review your code and to ensure that it works as expected. +Lastly the changes are tested. If it doesn't work out of the box, we'll ask your to review your code and to ensure that it works as expected. It's totally normal for a PR to need some more work even after it was created, that's why we review them. But every round trip takes time, so it's good practice to try to reduce them as much as possible by following those simple rules. - -# Coding convention - -## Language - -The language of this project is **C++**, and all new code must be written in C++. (Modern) C++ provides a lot of useful tools and functionalities that are beneficial for embedded software development like `constexpr`, `template` and anything that provides zero-cost abstraction. - -C code is accepted if it comes from another library like FreeRTOS, NimBLE, LVGL or the NRF-SDK. - -## Coding style - -The most important rule to follow is to try to keep the code as easy to read and maintain as possible. - -Using an autoformatter is highly recommended, but make sure it's configured properly. - -There are preconfigured autoformatter rules for: - - * CLion (IntelliJ) in .idea/codeStyles/Project.xml - -If there are no preconfigured rules for your IDE, you can use one of the existing ones to configure your IDE. - - - **Indentation** : 2 spaces, no tabulation - - **Opening brace** at the end of the line - - **Naming** : Choose self-describing variable name - - **class** : PascalCase - - **namespace** : PascalCase - - **variable** : camelCase, **no** prefix/suffix ('_', 'm_',...) for class members - - **Include guard** : `#pragma once` (no `#ifdef __MODULE__ / #define __MODULE__ / #endif`) - - **Includes** : - - files from the project : `#include "relative/path/to/the/file.h"` - - external files and std : `#include ` - - Only use [primary spellings for operators and tokens](https://en.cppreference.com/w/cpp/language/operator_alternative) - - Use auto sparingly. Don't use auto for [fundamental/built-in types](https://en.cppreference.com/w/cpp/language/types) and [fixed width integer types](https://en.cppreference.com/w/cpp/types/integer), except when initializing with a cast to avoid duplicating the type name. - - Examples: - - `auto* app = static_cast(instance);` - - `auto number = static_cast(variable);` - - `uint8_t returnValue = MyFunction();` - - Use nullptr instead of NULL -- cgit v1.2.3-70-g09d2 From caec4a560b46f30f08f03ea5cff4c902034956e4 Mon Sep 17 00:00:00 2001 From: Riku Isokoski Date: Mon, 8 Nov 2021 19:14:23 +0200 Subject: Replace some "we" --- doc/contribute.md | 6 +++--- doc/versioning.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'doc/contribute.md') diff --git a/doc/contribute.md b/doc/contribute.md index 70fc567c..b1be84a4 100644 --- a/doc/contribute.md +++ b/doc/contribute.md @@ -36,7 +36,7 @@ When your feature branch is ready, **make sure it actually works** and **do not **Creating a pull request containing modifications that haven't been tested is strongly discouraged.** If for any reason you cannot test your modifications, but want to publish them anyway, **please mention it in the description**. This way, other contributors might be willing to test it and provide feedback about your code. -Before submitting a PR, check your code against our [coding conventions](/doc/coding-convention.md). This project also provides [clang-format](../.clang-format) and [clang-tidy](../.clang-tidy) configuration files. You should use them to ensure correct formatting of your code. +Before submitting a PR, check your code against the [coding conventions](/doc/coding-convention.md). This project also provides [clang-format](../.clang-format) and [clang-tidy](../.clang-tidy) configuration files. You should use them to ensure correct formatting of your code. Don't forget to check the files you are going to commit and remove those which aren't necessary (config files from your IDE, for example). Remove old comments, commented code,... @@ -54,8 +54,8 @@ Reviewers will first look at the **description**. If it's easy to understand wha Reviewing **a few files that were modified for a single purpose** is a lot easier than reviewing 30 files modified for many reasons (bug fix, UI improvements, typos in doc,...), even if all the changes make sense. Also, it's possible that we agree on some modification but not on another, so we won't be able to merge the PR because of the changes that are not accepted. -We do our best to keep the code as consistent as possible. If the formatting of your code is not consistent with our code base, we'll ask you to review it. +The code base should be kept as consistent as possible. If the formatting of your code is not consistent with the rest of the code base, we'll ask you to review it. -Lastly the changes are tested. If it doesn't work out of the box, we'll ask your to review your code and to ensure that it works as expected. +Lastly the changes are tested. If it doesn't work out of the box, we'll ask you to review your code and to ensure that it works as expected. It's totally normal for a PR to need some more work even after it was created, that's why we review them. But every round trip takes time, so it's good practice to try to reduce them as much as possible by following those simple rules. diff --git a/doc/versioning.md b/doc/versioning.md index 8b87d473..2fa36ab9 100644 --- a/doc/versioning.md +++ b/doc/versioning.md @@ -1,6 +1,6 @@ # Versioning The versioning of this project is based on [Semantic versioning](https://semver.org/): - - The **patch** is incremented when we fix a bug on a **released** version (most of the time using a **hotfix** branch). - - The **minor** is incremented when we release a new version with new features. It corresponds to a merge of **develop** into **master**. + - The **patch** is incremented when a bug is fixed on a **released** version (most of the time using a **hotfix** branch). + - The **minor** is incremented when a new version with new features is released. It corresponds to a merge of **develop** into **master**. - The **major** should be incremented when a breaking change is made to the application. We still have to define what is a breaking change in the context of this project. -- cgit v1.2.3-70-g09d2 From f66fcdd3ca64fcbecfa264a506b7973b47ba1213 Mon Sep 17 00:00:00 2001 From: Eli Weiss Date: Mon, 29 Nov 2021 21:51:35 -0600 Subject: Improved documentation readability Removed extra space in contribute.md and cleaned up MemoryAnalysis.md phrasing and confusing punctuation. --- doc/MemoryAnalysis.md | 18 +++++++++--------- doc/contribute.md | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'doc/contribute.md') diff --git a/doc/MemoryAnalysis.md b/doc/MemoryAnalysis.md index 7304e3f3..20bb9283 100644 --- a/doc/MemoryAnalysis.md +++ b/doc/MemoryAnalysis.md @@ -32,13 +32,13 @@ In this analysis, I used [Linkermapviz](https://github.com/PromyLOPh/linkermapvi ### Linkermapviz -[Linkermapviz](https://github.com/PromyLOPh/linkermapviz) parses the MAP file and displays its content in a graphical way into an HTML page: +[Linkermapviz](https://github.com/PromyLOPh/linkermapviz) parses the MAP file and displays its content on an HTML page as a graphic: ![linkermapviz](./memoryAnalysis/linkermapviz.png) -Using this tool, you can easily see the size of each symbol relative to the other one, and check what is using most of the space,... +Using this tool, you can compare the relative size of symbols. This can be helpful for checking memory usage at a glance. -Also, as Linkermapviz is written in Python, you can easily modify it to adapt it to your firmware, export data in another format,... For example, [I modified it to parse the contents of the MAP file and export it in a CSV file](https://github.com/InfiniTimeOrg/InfiniTime/issues/313#issuecomment-842338620). I could later on open this file in LibreOffice Calc and use sort/filter functionality to search for specific symbols in specific files... +Also, as Linkermapviz is written in Python, you can easily modify and adapt it to your firmware or export data in another format. For example, [here it is modified to parse the contents of the MAP file and export it in a CSV file](https://github.com/InfiniTimeOrg/InfiniTime/issues/313#issuecomment-842338620). This file could later be opened in LibreOffice Calc where sort/filter functionality could be used to search for specific symbols in specific files... ### Puncover [Puncover](https://github.com/HBehrens/puncover) is another useful tools that analyses the binary file generated by the compiler (the .out file that contains all debug information). It provides valuable information about the symbols (data and code): name, position, size, max stack of each functions, callers, callees... @@ -46,8 +46,8 @@ Also, as Linkermapviz is written in Python, you can easily modify it to adapt it Puncover is really easy to install: - - clone the repo and cd into the cloned directory - - setup a venv + - Clone the repo and cd into the cloned directory + - Setup a venv - `python -m virtualenv venv` - `source venv/bin/activate` - Install : `pip install .` @@ -60,13 +60,13 @@ Puncover is really easy to install: - Launch a browser at http://localhost:5000/ ### Analysis -Using the MAP file and tools, we can easily see what symbols are using most of the FLASH memory space. In this case, with no surprise, fonts and graphics are the biggest flash space consumer. +Using the MAP file and tools, we can easily see what symbols are using most of the flash memory. In this case, with no surprise, fonts and graphics are the largest use of flash memory. ![Puncover](./memoryAnalysis/puncover-all-symbols.png) -This way, you can easily check what needs to be optimized : we should find a way to store big static data (like fonts and graphics) in the external flash memory, for example. +This way, you can easily check what needs to be optimized. We should find a way to store big static data (like fonts and graphics) in the external flash memory, for example. -It's always a good idea to check the flash memory space when working on the project : this way, you can easily check that your developments are using a reasonable amount of space. +It's always a good idea to check the flash memory space when working on the project. This way, you can easily check that your developments are using a reasonable amount of space. ### Links - Analysis with linkermapviz : https://github.com/InfiniTimeOrg/InfiniTime/issues/313#issuecomment-842338620 @@ -210,7 +210,7 @@ NRF_LOG_INFO("heap : %d", m.uordblks); ``` #### Analysis -According to my experimentation, InfiniTime uses ~6000bytes of heap most of the time. Except when the Navigation app is launched, where the heap usage increases to... more than 9500 bytes (meaning that the heap overflows and could potentially corrupt the stack!!!). This is a bug that should be fixed in #362. +According to my experimentation, InfiniTime uses ~6000bytes of heap most of the time. Except when the Navigation app is launched, where the heap usage exceeds 9500 bytes (meaning that the heap overflows and could potentially corrupt the stack). This is a bug that should be fixed in #362. To know exactly what's consuming heap memory, you can `wrap` functions like `malloc()` into your own functions. In this wrapper, you can add logging code or put breakpoints: diff --git a/doc/contribute.md b/doc/contribute.md index b1be84a4..f2a4aeaa 100644 --- a/doc/contribute.md +++ b/doc/contribute.md @@ -46,7 +46,7 @@ Other contributors can post comments about the pull request, maybe ask for more Once the pull request is reviewed and accepted, it'll be merged into **develop** and will be released in the next version of the firmware. -## Why all these rules? +## Why all these rules? Reviewing pull requests is a **very time consuming task**. Everything you do to make reviewing easier will **get your PR merged faster**. -- cgit v1.2.3-70-g09d2