Replaced byte with uint8_t in the arduino files. #43
Conversation
|
@SaarYogev, thanks. I'll see if I can look at it over the weekend, or maybe @nimrod46 can. Can you do me a favor and run the auto-formatter too? Follow the instructions here: https://github.com/AmboVent-1690-108/AmboVent#software. I want to make sure they are easy enough to follow for you to get it to work. |
|
Sure, I'll try it tomorrow. By the way, is auto-formatting an issue already? If so just mention it, if not I'll open one. Also, we need to include it in CI ( #42 ). |
|
@ElectricRCAircraftGuy $ ./run_clang-format.sh
THIS_DIR = "..../AmboVent"
FILE_LIST = "..../AmboVent/3-Software/Arduino/ventilation_machine/ventilation_machine.ino"
Formatting ..../AmboVent/3-Software/Arduino/ventilation_machine/ventilation_machine.ino
YAML:184:22: error: unknown key 'Delimiter'
- Delimiter: pb
^~
Error reading ..../AmboVent/3-Software/Arduino\.clang-format: invalid argumentWhere |
|
Is that holding this PR back? If not, it's probably better to open a separate issue on making sure |
| @@ -275,7 +275,7 @@ void Servo::write(int value) | |||
| void Servo::writeMicroseconds(int value) | |||
| { | |||
| // calculate and store the values for the given channel | |||
| byte channel = this->servoIndex; | |||
| uint8_t channel = this->servoIndex; | |||
There was a problem hiding this comment.
Let's not change anything in the core libraries. Please remove all changes to Libraries/Servo. Otherwise we make our libraries deviate from the Arduino core with no real differences, and it adds work when the core libraries change or get upgraded.
Yes, we don't want to land any future PRs without running the clang formatter first on the core AmboVent files (this doesn't include Libraries we didn't write--we won't format those). Please open an issue and I'd love to help you debug getting the formatter to work. The variable renames here are trivial so I'll just catch them real quick. Getting the formatter to work on anyone's system who wants to merge a PR is more important to me, however, so let's work that out if you are willing to. I want to ensure we have instructions that are easy to follow. |
…t8_t` Cleaning up the #defines included: 1. renaming to have more descriptive names, names in ALL_CAPITALS, and names which ended in their units (ex: _PA for Pascals) 2. Changing all 0s meaning false to `false`, all 1s meaning true to `true` 3. Adding more detailed descriptions, such as for the low-pass-filter IIR (Infinite Impulse Response) alpha constants When converting `byte` --> `uint8_t`, I also changed the parenthesis style from `uint8_t(val)`, which mistakenly looks like a function, to `(uint8_t)val`, which is clearly a C-style type cast. This closes AmboVent-1690-108#43 (a PR), since it replaces it.
|
@SaarYogev , no need to fix any of this anymore. #46 does a ton of other stuff and also replaces this PR while I'm at it. I'd still like to get your clang-format tool working, however, so I'd be happy to help you debug if you are willing to open an issue. We need to make that process smooth, as running the formatter is a requirement to land code. |
Agreed, we need to get the auto-formater to run in CI if possible. I don't think we have an open issue to integrate the auto-formatter with CI. Feel free to open an issue for that. I only had one to get the auto-formatter working manually I think. Yes, see #21. It's closed. |
…t8_t` Cleaning up the #defines included: 1. renaming to have more descriptive names, names in ALL_CAPITALS, and names which ended in their units (ex: _PA for Pascals) 2. Changing all 0s meaning false to `false`, all 1s meaning true to `true` 3. Adding more detailed descriptions, such as for the low-pass-filter IIR (Infinite Impulse Response) alpha constants When converting `byte` --> `uint8_t`, I also changed the parenthesis style from `uint8_t(val)`, which mistakenly looks like a function, to `(uint8_t)val`, which is clearly a C-style type cast. This closes AmboVent-1690-108#43 (a PR), since it replaces it.
Deals with the additional part of #32 (not the sub issue of #11, that'll be in a future PR).