Отрецензируйте код
- Войдите на сайт для отправки комментариев
Пт, 22/04/2016 - 19:20
Всем привет! Просьба к бывалым. Отрецензируйте мой код, или укажите на ошибки. Раньше программировал только в школе). Код писал сам. проверил в железе работает. Теперь к сути. есть четыре входа и три выхода. Ко входам подключены 4 кнопки. Входы управляются именно плюсом. на выходы подключено 3 ИМ. При нажатии с 1 по 3 кнопку включается соотв. выход. при нажатии 4 кнопки все выходы сбрасываются в 0. так же есть пернеход из пред состояние в любое другое без сброса. В коде реализовал защиту от дребезга контактов хоть и не как в примерах, потому что в примерах debounce пока не разобрался.
// 3-х позиционный переключатель через 3 реле const int buttonPin2 = 2; // входы const int buttonPin3 = 3; const int buttonPin4 = 4; const int buttonPin5 = 5; const int ledPin2 = 12; // выходы const int ledPin3 = 13; const int ledPin4 = 11; int buttonState = 0; // начальное сост кнопок void setup() { pinMode(ledPin2, OUTPUT); // настройка пинов на выход pinMode(ledPin3, OUTPUT); pinMode(ledPin4, OUTPUT); pinMode(buttonPin2, INPUT); // настройка пинов на вход pinMode(buttonPin3, INPUT); pinMode(buttonPin4, INPUT); pinMode(buttonPin5, INPUT); } void loop() { buttonState = digitalRead(buttonPin2); //position 1 if (buttonState == HIGH){ delay(10); } buttonState = digitalRead(buttonPin2); if (buttonState == HIGH){ digitalWrite(ledPin2, HIGH); digitalWrite(ledPin3, LOW); digitalWrite(ledPin4, LOW); } buttonState = digitalRead(buttonPin3); //position 2 if (buttonState == HIGH){ delay(10); } buttonState = digitalRead(buttonPin3); if (buttonState == HIGH){ digitalWrite(ledPin2, LOW); digitalWrite(ledPin3, HIGH); digitalWrite(ledPin4, LOW); } buttonState = digitalRead(buttonPin4); //position 3 if (buttonState == HIGH){ delay(10); } buttonState = digitalRead(buttonPin4); if (buttonState == HIGH){ digitalWrite(ledPin2, LOW); digitalWrite(ledPin3, LOW); digitalWrite(ledPin4, HIGH); } buttonState = digitalRead(buttonPin5); //position 3 if (buttonState == HIGH){ delay(10); } buttonState = digitalRead(buttonPin5); if (buttonState == HIGH){ digitalWrite(ledPin2, LOW); digitalWrite(ledPin3, LOW); digitalWrite(ledPin4, LOW); } }
Ваша "защита от дребезга" выглядела бы намного логичнее, если перенести одну "}":
Строки
Отрецензируйте мой код, или укажите на ошибки.
Вы действительно этого хотите? Объективной рецензии, а не простого "одобрмяс"?
Вот честно, я готов потратить час другой на это, при условии, что Вам именно это нужно. А то у меня уже есть печальный опыт, когда пластался для человека, но моя рецензия ему не понравилась, в итоге, начитался гору мата и всё о себе узнал :)
Так оно Вам реально надо? Объективно, а не "похвалить"?
Спасибо откликнувшимся, но хотелось бы видеть объективную критику с объяснениями ошибок а не "код гавно" от родившихся программистами. есть че по делу сказать и не жалко - спасибо. а задачи написать первый код сверхгладким не стояло. главное что работает в железе. единственное хотелось бы знать глюки кода на будущее, потому что обслуживание устройства будет крайне затруднительным. а над озвученными предложениями я подумаю. спасибо. PS. задачи получить похвалу тоже не стояло. пока делаю для себя. а разобраться хочется. на достигнутом хоть и кривом останавливаться не собираюсь. хотелось бы подробнее про свич. один порт подразумевает один пин?. каким образом опрос всех кноопок?
Спасибо откликнувшимся, но хотелось бы видеть объективную критику с объяснениями ошибок а не "код гавно" от родившихся программистами.
я же тебе показал на ошибку - сделай без делаев.
и, я не программист.
без delay можно сделать только как в образце debounce. я в нем пока не разобрался с булевой переменной. а другого варианта я не придумал. а что именно плохого в delay?
без delay можно сделать только как в образце debounce. я в нем пока не разобрался с булевой переменной. а другого варианта я не придумал. а что именно плохого в delay?
ок. представь, что ты водитель автомобиля и твой мозг работает ровно так, как и твой код - опрос рецепторов твоего организма происходит не со скоростью движения электронов по нервным волокнам, а с задержкой 10 милисекунд после опроса одного рецептора. рецепторов у тебя, допустим 10000, т.е. для того, что бы тебе почесать нос, а затем, оценить цвет сигнала светофора, может пройти 100 секунд. что бы затем нажать на педаль тормоза, может пройти 100 секунд.
кароче, если у тебя затем возникнет желание пользоваться девайсом, который не может вовремя реагировать на события, то ничего страшного, но в реале тебя выбесят такие кнопки.
Ну, давайте, помолясь, начнём. Только сразу предупреждаю, будет реальный разбор, а не «одобрямс». И ещё, мне чертовски приятно, когда люди сами пытаются программировать, и таким людям я стараюсь, по возможности, помочь. Поэтому, если какое-то моё слово или выражение покажется Вам обидным – не берите в голову, просто не так выразился – абсолютно не хочу Вас обидеть, действительно хочу помочь, на то и трачу время. Хотел бы обидеть – написал бы одну строку про «говнокод» и успокоился бы.
Поехали.
Про дребезг
Обработка дребезга у Вас не просто не работает, а наоборот привносит дребезг. Т.е. при Вашей методике обработки, даже если кнопка нажмётся вовсе без дребезга (аппаратно подавлен), в программе дребезг всё равно будет присутствовать. Т.е. вместо подавления дребезга, Вы привносите его.
Сейчас мы разберёмся почему, но сначала о замечаниях коллег.
Слово «delay» на этом форуме считается матерным и потому на Вас так набросились. На самом деле, оно вовсе не матерное, если его использовать с умом и к месту. В Вашем случае в нём ничего страшного нет, т.к. эти задержки ни на что особо не влияют. Например, известный всему ардуино-миру «говнокодер» Джереми Блум, в своей книге «Изучаем Ардуино: инструменты и методы технического волшебства» приводит вот такую программу подавления дребезга:
(замечание: именно так программа выглядит в кодах, приведённых на сайте поддержки книги. В бумажном же издании (стр. 34) была допущена опечатка – пропустили строку 6. А в русском бумажном издании (стр. 55-56) решили исправить и вставили её только не на место, а поменяв местами со строкой 7 - в итоге бред)
Конечно, Дж. Блум бы сильно расстроился, узнав мнение г-на Клапауция о своём коде, но, слава Богу, он о нём вряд ли узнает.
Теперь давайте сравним этот код с Вашим и разберёмся почему код Блума фильтрует дребезг, а Ваш - наоборот, его привносит.
Обратите внимание, что Блум проверяет предыдущее состояние кнопки и сравнивает его с текущим. В остальном коде (снаружи этой функции) он принимает решение о том, что кнопка нажата / отпущена не по факту HIGH / LOW на ней, а по факту изменение состояния пина с HIGH на LOW или наоборот. Вы же не смотрите на предыдущее состояние кнопки, а смотрите только на то HIGH там или LOW и всё.
Что в итоге получается? Допустим, я нажал кнопку buttonPin2 (считаем, что никакого дребезга нет вовсе – подавлен аппаратно). Вы в строках 25-30 обнаружили на ней HIGH и выполнили строки 31-33. Через очень короткое время loop вызвался ещё раз (кнопка всё ещё нажата!). Вы снова в строках 25-30 обнаружили на ней HIGH и выполнили строки 31-33. Затем ещё раз! Среднее время нажатия кнопки человеком, который не стремится ни подержать её подольше, ни отпустить как можно быстрее, примерно 50-60мс. За это время loop успеет вызваться раз 6-7. Таким образом, строки 31-33 будут выполнены 6-7 раз за одно нажатие кнопки! Понятно теперь почему я говорю, что Ваш алгоритм привносит дребезг, а не подавляет его? Если непонятно – спрашивайте, не стесняйтесь.
Почему же у Вас всё «на вид» нормально работает? Да, просто потому, что в строках 31-33 Вы просто назначаете пинам HIGH / LOW и ничего больше. Ну, сделаете Вы это не один раз, а 6-7 раз – хуже никому не будет. Вот если бы Вы в этих строках, например, инвертировали какой-нибудь пин – вот тут то Вы этот дребезг заметили бы! Если хотите, можете поставить туда счётчик «нажатий» и убедиться, что всё работает так, как я описал.
Из последнего абзаца можно сделать интересный вывод. При Вашем алгоритме обработки кнопок, Вам дребезг абсолютно пофигу и в данной программе его можно не обрабатывать вовсе – просто не париться. Ничего не изменится. Ну, «дребезгнёт» она, и что? Лишний раз те же состояния пинам назначите, так Вы и сейчас это делаете.
(замечание: есть «родственное» дребезгу явление – наносекундные помехи. Например, Вы не прикасались к кнопке, а из-за помехи по питанию в пин прилетел HIGH на короткое (типа наносекунды) время. Такие вещи, конечно, важны и для Вашего алгоритма обработки. Борются с наносекундными помехами также как с дребезгом (проверяют, сохранился ли HIGH по истечении какого-то временного интервала). Однако если пин притянут к земле или питанию, с питанием более или менее всё в порядке, и Ваша программа не предназначена для управления атомным реактором, то на наносекундные помехи можно плюнуть и не париться).
Выводы по дребезгу:
1. при том алгоритме, что у Вас дребезг не подавляется, а наоборот привносится;
2. при Вашем алгоритме обработки кнопок, всякую борьбу с дребезгом можно убрать – она попросту не нужна;
2. если впредь захотите бороться с дребезгом, то для программ в которых небольшие задержки не мешают, используйте подход Блума, но обязательно фиксируйте факт изменение состояния пина кнопки, а не только то, что на нём HIGH. Для программ же, в которых задержки нежелательны, избегайте delay. Лучше всего Вам не разбирать готовую библиотеку debounce, а почитать замечательную статью на эту тему. Понимания будет гораздо больше.
Про глобальную переменную
В строке 10 у Вас описана глобальная переменная, которая используется в одной единственной функции – loop().
Это очень плохая практика. Такие переменные грамотнее описывать внутри функции, а для того чтобы она сохраняла значение между вызовами, описывать её как static. Т.е. вместо строки 10, Вам нужно внутри функции loop описать
static int buttonState = 0;
По занимаемой памяти, скорости доступа, по всему – это ровно тоже самое. Т.е. от того, что она у Вас глобальная. Вы не выигрываете ровным счётом ничего! И ничего не выигрывая, Вы наживаете себе некоторые риски, а именно: в большой программе Вы можете случайно изменить эту переменную вне функции loop и это сломает Вашу логику. Причём поменять её Вы можете даже из другого файла программы (она доступна отовсюду).
Про повторяющиеся куски кода
Практически один и тот же кусок кода (типа как в строках 31-33) у Вас повторяется четыре раза. Это тоже очень плохая практика. Такие куски надо оформлять функциями. Здесь правда есть нюанс. В программе типа Вашей, можно не париться, сделать функцию с тремя параметрами и использовать.
Но в критичных по памяти / скорости кодах всегда надо помнить о компромиссе скорость / память. Повторить код четыре раза даёт небольшой выигрыш по времени по сравнению с использованием функции. Зато использование функции даёт выигрыш по памяти.
Как быть? Делать функцию или таки нет?
Ответ – функцию надо делать в любом случае, т.к. это улучшает читабельность программы. Но при этом, если критична скорость, то перед описанием функции следует добавить ключевое слово inline. Это заставит компилятор не создавать функцию отдельным куском кода, а развернуть её код непосредственно в точке(ах) вызова. Т.е. в итоге Вы получите такой же код как сейчас, но "на вид" это будет функция, ничего 4 раза не повторяется и читабельность повысится.
Чисто по стилистике
Вам самому будет легче, если переменные будут называться не buttonPin2 и ledPin2 а как-нибудь более привязанно к их функциональному назначению. Например, criticalPressureIndicatorLed или ещё как.
Евгений, большое спасибо за очень развернутый ответ! Идею Клапауция я тоже понял. Но ваш ответ настолько большой что читал его несколько раз. и опять не все понял. про привносимый дребезг понятно( в этом цикле невозможно точно предсказать количество выполнения 31-33 строк и т.д. я правильно понял?) статья может конечно и полезная но знаний англ не хватает чтобы ее прочитать как научную. а пользоваться кривым переводом не вижу смысла. дальше про Блума. на его код стоит ориентироваться или все таки забыть о delay? просто не до конца понятем механизм ( а так же его написание) опрса состояние кнопок. а именно сравнение пред и посл состояния. и как это сделать без delay? нужна какоя то же пауза для повторного опроса. и почему в моем коде защита эта не нужна? еще есть много вопросов. но пока остановился бы на наиболее важных на данный момент. Еще раз СПАСИБО!
Metalluga, по поводу отказа от delay() и чем его заменить. В Arduino IDE есть пример BlinkWithoutDalay, ещё почитай:
http://arduino.ru/tutorials/BlinkWithoutDelay
http://robocraft.ru/blog/arduino/385.html
хоть код по ссылкам и одинаковый но из второй ссылки понятнее. Евгений, оцените пожалуйста такую дорботку без delay. хотя чую вней тоже что то кривое есть. но совместными усилиями надеюсь побороть)) привел часть кода чтобы не городить.
но почитал комментарии с примера. и есть вопрос перепоолнением счетчика. как решить пока не понял. и хотелось бы упростить код используя логические операции но как??
По-моему Вы запутались совсем.
В приведенном коде:
1. 41-43 строки срабатывают вне зависимости от состояния кнопки, так?
2. 34-37 строка - крутимся тут пока не вывалимся по превышению interval, так? И что? прочитали состояние кнопки и дальше пошли. А проверка состояния?
3. 34-37 - получили почти тотже результат что и с delay - пока тут крутимся другой код не выполняем:(
Идея такая:
1. Чиатем состояние. Запоминаем, время1 и состояние1. Идем дальше
2. Проверяем millis(). Если больше времени1 + интервал, то проверяем кнопку еще раз. Если нужное нам состояние (состояние2=состояние1 - считаем что кнопка находится в этом состоянии по двум временным отметкам) - то выполяем действия.
сам вижу что опять что то не так но не пойму. получается цикл в цикле. и без перехода опять никак. а это опять намотка бесполезных тактов.. как одновременно проверить и millis и условие совпадения состояний? и еще. запимнить состояние вроде понятно. а как запомнить время?
а в 13 посте вроде строки 41-43 в теле цикла и по идее не должны выполняться при любом условии?
как одновременно проверить и millis и условие совпадения состояний?
читаем, читаем и еще раз читаем http://arduino.ru/Reference/Boolean
апимнить состояние вроде понятно. а как запомнить время?
А PreviosMillis у Вас что делает?
а в 13 посте вроде строки 41-43 в теле цикла и по идее не должны выполняться при любом условии?
Насколько я вижу они за пределами всех If-ов , в теле loop.
Теперь о коде #15:
1. 24 строка - от чего проверяем интервал?
2. 26 - и какое состояние кнопки?
Отстранитесь от кода, выдохните, нарисуйте понятный Вам алгоритм, а потом уж за код!