From 0fc8e8d483bb4df1c0e1ca3b77d318476b5a6b99 Mon Sep 17 00:00:00 2001 From: Otto Winter Date: Sat, 25 Jul 2020 14:22:56 +0200 Subject: [PATCH] Fix SN74HC595 doesn't use ESPHome HAL and add lint checks for it (#1188) --- esphome/components/adc/adc_sensor.cpp | 4 +- .../components/max7219digit/max7219digit.cpp | 31 +++++----- esphome/components/mcp3008/mcp3008.cpp | 8 +-- esphome/components/sn74hc595/sn74hc595.cpp | 35 ++++++----- esphome/components/sx1509/sx1509.cpp | 2 +- .../ultrasonic/ultrasonic_sensor.cpp | 3 +- script/ci-custom.py | 59 ++++++++++++++++++- 7 files changed, 105 insertions(+), 37 deletions(-) diff --git a/esphome/components/adc/adc_sensor.cpp b/esphome/components/adc/adc_sensor.cpp index 2c448d0392..9b1f452131 100644 --- a/esphome/components/adc/adc_sensor.cpp +++ b/esphome/components/adc/adc_sensor.cpp @@ -58,7 +58,7 @@ void ADCSensor::update() { } float ADCSensor::sample() { #ifdef ARDUINO_ARCH_ESP32 - float value_v = analogRead(this->pin_) / 4095.0f; + float value_v = analogRead(this->pin_) / 4095.0f; // NOLINT switch (this->attenuation_) { case ADC_0db: value_v *= 1.1; @@ -80,7 +80,7 @@ float ADCSensor::sample() { #ifdef USE_ADC_SENSOR_VCC return ESP.getVcc() / 1024.0f; #else - return analogRead(this->pin_) / 1024.0f; + return analogRead(this->pin_) / 1024.0f; // NOLINT #endif #endif } diff --git a/esphome/components/max7219digit/max7219digit.cpp b/esphome/components/max7219digit/max7219digit.cpp index dff1d27370..ab9be4ad36 100644 --- a/esphome/components/max7219digit/max7219digit.cpp +++ b/esphome/components/max7219digit/max7219digit.cpp @@ -101,7 +101,7 @@ void MAX7219Component::loop() { } void MAX7219Component::display() { - byte pixels[8]; + uint8_t pixels[8]; // Run this loop for every MAX CHIP (GRID OF 64 leds) // Run this routine for the rows of every chip 8x row 0 top to 7 bottom // Fill the pixel parameter with diplay data @@ -205,29 +205,32 @@ void MAX7219Component::scroll_left() { this->stepsleft_++; } -void MAX7219Component::send_char(byte chip, byte data) { +void MAX7219Component::send_char(uint8_t chip, uint8_t data) { // get this character from PROGMEM - for (byte i = 0; i < 8; i++) + for (uint8_t i = 0; i < 8; i++) this->max_displaybuffer_[chip * 8 + i] = pgm_read_byte(&MAX7219_DOT_MATRIX_FONT[data][i]); } // end of send_char // send one character (data) to position (chip) -void MAX7219Component::send64pixels(byte chip, const byte pixels[8]) { - for (byte col = 0; col < 8; col++) { // RUN THIS LOOP 8 times until column is 7 - this->enable(); // start sending by enabling SPI - for (byte i = 0; i < chip; i++) // send extra NOPs to push the pixels out to extra displays +void MAX7219Component::send64pixels(uint8_t chip, const uint8_t pixels[8]) { + for (uint8_t col = 0; col < 8; col++) { // RUN THIS LOOP 8 times until column is 7 + this->enable(); // start sending by enabling SPI + for (uint8_t i = 0; i < chip; i++) // send extra NOPs to push the pixels out to extra displays this->send_byte_(MAX7219_REGISTER_NOOP, MAX7219_REGISTER_NOOP); // run this loop unit the matching chip is reached - byte b = 0; // rotate pixels 90 degrees -- set byte to 0 + uint8_t b = 0; // rotate pixels 90 degrees -- set byte to 0 if (this->orientation_ == 0) { - for (byte i = 0; i < 8; i++) // run this loop 8 times for all the pixels[8] received - b |= bitRead(pixels[i], col) << (7 - i); // change the column bits into row bits + for (uint8_t i = 0; i < 8; i++) { + // run this loop 8 times for all the pixels[8] received + b |= ((pixels[i] >> col) & 1) << (7 - i); // change the column bits into row bits + } } else if (this->orientation_ == 1) { b = pixels[col]; } else if (this->orientation_ == 2) { - for (byte i = 0; i < 8; i++) - b |= bitRead(pixels[i], 7 - col) << (7 - i); + for (uint8_t i = 0; i < 8; i++) { + b |= ((pixels[i] >> (7 - col)) << (7 - i)); + } } else { b = pixels[7 - col]; } @@ -246,8 +249,8 @@ void MAX7219Component::send64pixels(byte chip, const byte pixels[8]) { uint8_t MAX7219Component::printdigit(const char *str) { return this->printdigit(0, str); } uint8_t MAX7219Component::printdigit(uint8_t start_pos, const char *s) { - byte chip; - for (chip = start_pos; chip < this->num_chips_ && *s; chip++) + uint8_t chip = start_pos; + for (; chip < this->num_chips_ && *s; chip++) send_char(chip, *s++); // space out rest while (chip < (this->num_chips_)) diff --git a/esphome/components/mcp3008/mcp3008.cpp b/esphome/components/mcp3008/mcp3008.cpp index 70484e8818..a4d019ab8f 100644 --- a/esphome/components/mcp3008/mcp3008.cpp +++ b/esphome/components/mcp3008/mcp3008.cpp @@ -19,11 +19,11 @@ void MCP3008::dump_config() { } float MCP3008::read_data_(uint8_t pin) { - byte data_msb = 0; - byte data_lsb = 0; + uint8_t data_msb = 0; + uint8_t data_lsb = 0; - byte command = ((0x01 << 7) | // start bit - ((pin & 0x07) << 4)); // channel number + uint8_t command = ((0x01 << 7) | // start bit + ((pin & 0x07) << 4)); // channel number this->enable(); diff --git a/esphome/components/sn74hc595/sn74hc595.cpp b/esphome/components/sn74hc595/sn74hc595.cpp index edf8989149..d82c5d5036 100644 --- a/esphome/components/sn74hc595/sn74hc595.cpp +++ b/esphome/components/sn74hc595/sn74hc595.cpp @@ -10,17 +10,17 @@ void SN74HC595Component::setup() { ESP_LOGCONFIG(TAG, "Setting up SN74HC595..."); if (this->have_oe_pin_) { // disable output - pinMode(this->oe_pin_->get_pin(), OUTPUT); - digitalWrite(this->oe_pin_->get_pin(), HIGH); + this->oe_pin_->pin_mode(OUTPUT); + this->oe_pin_->digital_write(true); } // initialize output pins - pinMode(this->clock_pin_->get_pin(), OUTPUT); - pinMode(this->data_pin_->get_pin(), OUTPUT); - pinMode(this->latch_pin_->get_pin(), OUTPUT); - digitalWrite(this->clock_pin_->get_pin(), LOW); - digitalWrite(this->data_pin_->get_pin(), LOW); - digitalWrite(this->latch_pin_->get_pin(), LOW); + this->clock_pin_->pin_mode(OUTPUT); + this->data_pin_->pin_mode(OUTPUT); + this->latch_pin_->pin_mode(OUTPUT); + this->clock_pin_->digital_write(LOW); + this->data_pin_->digital_write(LOW); + this->latch_pin_->digital_write(LOW); // send state to shift register this->write_gpio_(); @@ -28,26 +28,33 @@ void SN74HC595Component::setup() { void SN74HC595Component::dump_config() { ESP_LOGCONFIG(TAG, "SN74HC595:"); } -bool SN74HC595Component::digital_read_(uint8_t pin) { return bitRead(this->output_bits_, pin); } +bool SN74HC595Component::digital_read_(uint8_t pin) { return this->output_bits_ >> pin; } void SN74HC595Component::digital_write_(uint8_t pin, bool value) { - bitWrite(this->output_bits_, pin, value); + uint32_t mask = 1UL << pin; + this->output_bits_ &= ~mask; + if (value) + this->output_bits_ |= mask; this->write_gpio_(); } bool SN74HC595Component::write_gpio_() { for (int i = this->sr_count_ - 1; i >= 0; i--) { uint8_t data = (uint8_t)(this->output_bits_ >> (8 * i) & 0xff); - shiftOut(this->data_pin_->get_pin(), this->clock_pin_->get_pin(), MSBFIRST, data); + for (int j = 0; j < 8; j++) { + this->data_pin_->digital_write(data & (1 << (7 - j))); + this->clock_pin_->digital_write(true); + this->clock_pin_->digital_write(false); + } } // pulse latch to activate new values - digitalWrite(this->latch_pin_->get_pin(), HIGH); - digitalWrite(this->latch_pin_->get_pin(), LOW); + this->latch_pin_->digital_write(true); + this->latch_pin_->digital_write(false); // enable output if configured if (this->have_oe_pin_) { - digitalWrite(this->oe_pin_->get_pin(), LOW); + this->oe_pin_->digital_write(false); } return true; diff --git a/esphome/components/sx1509/sx1509.cpp b/esphome/components/sx1509/sx1509.cpp index 2806a1cac2..0d6ffbb9b8 100644 --- a/esphome/components/sx1509/sx1509.cpp +++ b/esphome/components/sx1509/sx1509.cpp @@ -144,7 +144,7 @@ void SX1509Component::clock_(byte osc_source, byte osc_pin_function, byte osc_fr osc_source = (osc_source & 0b11) << 5; // 2-bit value, bits 6:5 osc_pin_function = (osc_pin_function & 1) << 4; // 1-bit value bit 4 osc_freq_out = (osc_freq_out & 0b1111); // 4-bit value, bits 3:0 - byte reg_clock = osc_source | osc_pin_function | osc_freq_out; + uint8_t reg_clock = osc_source | osc_pin_function | osc_freq_out; this->write_byte(REG_CLOCK, reg_clock); osc_divider = constrain(osc_divider, 1, 7); diff --git a/esphome/components/ultrasonic/ultrasonic_sensor.cpp b/esphome/components/ultrasonic/ultrasonic_sensor.cpp index f8130f7d1f..c573c21863 100644 --- a/esphome/components/ultrasonic/ultrasonic_sensor.cpp +++ b/esphome/components/ultrasonic/ultrasonic_sensor.cpp @@ -17,7 +17,8 @@ void UltrasonicSensorComponent::update() { delayMicroseconds(this->pulse_time_us_); this->trigger_pin_->digital_write(false); - uint32_t time = pulseIn(this->echo_pin_->get_pin(), uint8_t(!this->echo_pin_->is_inverted()), this->timeout_us_); + uint32_t time = pulseIn( // NOLINT + this->echo_pin_->get_pin(), uint8_t(!this->echo_pin_->is_inverted()), this->timeout_us_); ESP_LOGV(TAG, "Echo took %uµs", time); diff --git a/script/ci-custom.py b/script/ci-custom.py index cde026b5d4..3954eea0de 100755 --- a/script/ci-custom.py +++ b/script/ci-custom.py @@ -255,6 +255,63 @@ def lint_conf_from_const_py(fname, match): "const.py directly.".format(highlight(name))) +RAW_PIN_ACCESS_RE = r'^\s(pinMode|digitalWrite|digitalRead)\((.*)->get_pin\(\),\s*([^)]+).*\)' + + +@lint_re_check(RAW_PIN_ACCESS_RE, include=cpp_include) +def lint_no_raw_pin_access(fname, match): + func = match.group(1) + pin = match.group(2) + mode = match.group(3) + new_func = { + 'pinMode': 'pin_mode', + 'digitalWrite': 'digital_write', + 'digitalRead': 'digital_read', + }[func] + new_code = highlight(f'{pin}->{new_func}({mode})') + return (f"Don't use raw {func} calls. Instead, use the `->{new_func}` function: {new_code}") + + +# Functions from Arduino framework that are forbidden to use directly +ARDUINO_FORBIDDEN = [ + 'digitalWrite', 'digitalRead', 'pinMode', + 'shiftOut', 'shiftIn', + 'radians', 'degrees', + 'interrupts', 'noInterrupts', + 'lowByte', 'highByte', + 'bitRead', 'bitSet', 'bitClear', 'bitWrite', + 'bit', 'analogRead', 'analogWrite', + 'pulseIn', 'pulseInLong', + 'tone', +] +ARDUINO_FORBIDDEN_RE = r'[^\w\d](' + r'|'.join(ARDUINO_FORBIDDEN) + r')\(.*' + + +@lint_re_check(ARDUINO_FORBIDDEN_RE, include=cpp_include, exclude=[ + 'esphome/components/mqtt/custom_mqtt_device.h', + 'esphome/core/esphal.*', +]) +def lint_no_arduino_framework_functions(fname, match): + nolint = highlight("// NOLINT") + return ( + f"The function {highlight(match.group(1))} from the Arduino framework is forbidden to be " + f"used directly in the ESPHome codebase. Please use ESPHome's abstractions and equivalent " + f"C++ instead.\n" + f"\n" + f"(If the function is strictly necessary, please add `{nolint}` to the end of the line)" + ) + + +@lint_re_check(r'[^\w\d]byte\s+[\w\d]+\s*=.*', include=cpp_include, exclude={ + 'esphome/components/tuya/tuya.h', +}) +def lint_no_byte_datatype(fname, match): + return ( + f"The datatype {highlight('byte')} is not allowed to be used in ESPHome. " + f"Please use {highlight('uint8_t')} instead." + ) + + @lint_post_check def lint_constants_usage(): errors = [] @@ -328,7 +385,7 @@ def lint_pragma_once(fname, content): return None -@lint_re_check(r'(whitelist|blacklist|slave)', exclude=['script/ci-custom.py'], +@lint_re_check(r'(whitelist|blacklist|slave)', exclude=['script/ci-custom.py'], flags=re.IGNORECASE | re.MULTILINE) def lint_inclusive_language(fname, match): # From https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb