search

AmboVent-1690-108 / AmboVent

AmboVent 1690.108
The Unlicense
205 stars 69 forks source link

Replaced byte with uint8_t in the arduino files. #43

Closed SaarYogev closed 4 years ago

SaarYogev commented 4 years ago

Deals with the additional part of #32 (not the sub issue of #11, that'll be in a future PR).

ElectricRCAircraftGuy commented 4 years ago

@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? 

./run_clang-format.sh

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.

SaarYogev commented 4 years ago

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

SaarYogev commented 4 years ago

@ElectricRCAircraftGuy ./run_clang-format.sh produces an error:

$ ./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 argument

Where .... is anything above the project's root directory.

SaarYogev commented 4 years ago

Is that holding this PR back? If not, it's probably better to open a separate issue on making sure run_clang-format.sh works.

ElectricRCAircraftGuy commented 4 years ago

@SaarYogev,

Is that holding this PR back? If not, it's probably better to open a separate issue on making sure run_clang-format.sh works.

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.

ElectricRCAircraftGuy commented 4 years ago

@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.

ElectricRCAircraftGuy commented 4 years ago

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

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.