Вопросы по С/С++ (СИ)

[ra9ust]

Продолжаем разбирать код, впрочем у меня оно уже полгода работает,и через полгода вернувшись к тексту всё понятно, дело привычки. Впрочем, все рекомендации принимаем к сведению.Уменьшив код,можно еще накидать дополнительных функций.

[Аlex]

Если работает - не трогайте. Не нужно ничего переделывать.
Все рекомендации и советы будете принимать во внимание при следующем проекте.

PS: Опыт приходит со временем.

[ra9ust]

Да почему же, действительно, в секундном прерывании я лишнего написал,поправил,проверил,если буду снимать устройство,или для будущего проекта,то уже с изменениями, исполнение сего кода у меня уменьшилось с 2.5мкс до 1мкс.

[viiv]

[uquote="ra9ust",url="/forum/viewtopic.php?p=3219963#p3219963"]вот прокомментируйте сей код, насколько он кошмарный/некошмарный[/uquote]

Вы сами легко прокомментируете данный код годика через два, если Вам захочется что-нибудь изменить/добавить.

[uquote="ra9ust",url="/forum/viewtopic.php?p=3219963#p3219963"]само устройство на нем работает как я и задумывал[/uquote]
«Не верю!» (с) Станиславский

Вы просто плохо тестировали устройство

Спойлер

Код: Выделить всё

if((day==32&&month==1)||(day==29&&month==2)||(day==32&&month==3)||(day==31&&month==4)||
 (day==32&&month==5)||(day==31&&month==6)||(day==32&&month==7)||(day==32&&month==8)||
 (day==31&&month==9)||(day==32&&month==10)||(day==31&&month==11)){month++;day=1;}
 if(day==32&&month==12){month=1;day=1;year++;} 

Вы специально сделали так, что в высокосные года Ваше устройство работало неправильно?

Спойлер

Код: Выделить всё

interrupt [TIM1_COMPA] void timer1_compa_isr(void)
{
 cc++;
 if(cc>=254)cc=14;
 sec++;
 if(sec>=60){min++;sec=0;}
 if(min>=60){hour++;min=0;tic=1;}
 if(hour==23&&min==59&&sec==50){if(cor>0)sec+=cor;} 
 if(hour==23&&min==59&&sec==59&&cc>11){if(cor<0)sec-=(128-(cor&0b01111111));cc=0;}
 if(hour>=24){day++;hour=0;week++;mint=0;maxt=0;
[b]/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */[/b]
eep_day=day;eep_month=month;eep_week=week;eep_year=year;}
 if(week>=8)week=1;       
 
 if((day==32&&month==1)||(day==29&&month==2)||(day==32&&month==3)||(day==31&&month==4)||
 (day==32&&month==5)||(day==31&&month==6)||(day==32&&month==7)||(day==32&&month==8)||
 (day==31&&month==9)||(day==32&&month==10)||(day==31&&month==11)){month++;day=1;}
 if(day==32&&month==12){month=1;day=1;year++;} 

 if(PINC.5==0&&w==1)secur++; 
 if(pirkor==1)pirkorr++;
 if(start_back==1)back--;
 if(start_rele==1)rele++; 
 countdown=((time_ohr-1)-rele); 
 if(start_mic==1)count_mic++;
 if(start_pirkor==1)count_pirkor++; 
 if(start_gerkon_1==1)count_gerkon_1++;
 if(start_gerkon_2==1)count_gerkon_2++;
 if(start_pirtua==1)count_pirtua++;
 
 if(ind_mic==1)cou++;
 if(start_ohtik==1)ohtik++;
 if(start_nochnik==1)tnochnik++;
 
 if(start_tt==1)tt--; 
 if(tt<0){timer_count--;tt=59;} 
 
 if(start_tt_2==1)tt_2--; 
 if(tt_2<0){timer_count_2--;tt_2=59;} 
 
 tal++;
 tr++;
 a++;
 b++;
 tpirr++;
 tpirrr++; 
}

Странно, eep_month и eep_year присваивается ДО корректировки month и year (т.е. возможно неправильными значениями).

[Пока_без_кота]

Еще вопрос о правилах "хорошего тона". Можно писАть

Код: Выделить всё

 if (delay != 0) delay --;

а можно

Код: Выделить всё

 if (delay) delay --;

Насколько эстетичны такие сокращения с точки зрения "профессиональных любителей" ? Ну типа, фыркаете ли Вы, видя такое в чужом коде, или нормально относитесь ?

[ra9ust]

Про високосные думал,но пока так оставил работать, насчет корректировки месяц и год,возможно,недочет.

[arkhnchul]

зависит от. Общее правило - если нас интересует числовое значение (как в данном примере), то и пишем явное сравнение с числом.

[ARV]

Ну типа, фыркаете ли Вы, видя такое в чужом коде, или нормально относитесь ?

дело не в том, фыркают ли другие... дело в том, видите ли вы полезный смысл в написанном или нет. у каждого своё понимание перфекционизма, но любой перфекционизм ради перфекционизма плох.

если вы НИКОГДА не будете проверять В ЭТОМ МЕСТЕ delay на равенство чему-то отличному от нуля, смысла писать != 0 нет. но если логика программы или избранного вами алгоритма такова, что ВОЗМОЖНЫ варианты, когда delay может сравниваться с чем-то еще - то стоит задуматься... а не правильнее ли написать delay != DELAY_LIMIT, определив DELAY_LIMIT равным 0, но подразумевая, что там может быть и нечто иное?

а вот -- или ++ я лично никогда не отделяю от операнда пробелом

Добавлено after 5 minutes 21 second:
[uquote="ra9ust",url="/forum/viewtopic.php?p=3219963#p3219963"]Ну хорошо, наверняка здесь есть хорошо владеющие СИ, вот прокомментируйте сей код, насколько он кошмарный/некошмарный, само устройство на нем работает как я и задумывал, но может как-то можно код сократить/улучшить без потери функциональности? Если кому понадобится могу скинуть весь проект+схема в протеусе. Кто что скажет?[/uquote]

[WiseLord]

Чёрт, да этот код от ra9ust в 100 раз круче регулярных выражений в плане write-only-code.

Добавлено after 11 minutes 25 seconds:
Ничего непонятно:

Спойлер

Код: Выделить всё

if((day==32&&month==1)||(day==29&&month==2)||(day==32&&month==3)||(day==31&&month==4)||
 (day==32&&month==5)||(day==31&&month==6)||(day==32&&month==7)||(day==32&&month==8)||
 (day==31&&month==9)||(day==32&&month==10)||(day==31&&month==11)){month++;day=1;}
 if(day==32&&month==12){month=1;day=1;year++;}

Просто переписано с отступами: уже понятно, что хотел автор, при этом хорошо видна ошибка с февралём в високосные годы

Спойлер

Код: Выделить всё

if ((day == 32 && month == 1) || (day == 29 && month == 2) ||
        (day == 32 && month == 3) || (day == 31 && month == 4) ||
        (day == 32 && month == 5) || (day == 31 && month == 6) ||
        (day == 32 && month == 7) || (day == 32 && month == 8) ||
        (day == 31 && month == 9) || (day == 32 && month == 10) ||
        (day == 31 && month == 11))
{
    month++;
    day = 1;
}
if (day == 32 && month == 12)
{
    month = 1;
    day = 1;
    year++;
}

А как бы такую логику делал я:

Спойлер

Код: Выделить всё

// Функция определяет число дней в месяце, уже с учётом високосных годов
static uint8_t DaysInMonth(void)
{
	uint8_t ret = month;
	if (ret == 2) {
		ret = year & 0x03;
		ret = (ret ? 29 : 28);
	} else {
		if (ret > 7)
			ret++;
		ret |= 30;
	}
	return ret;
}
// Дальше логика автора:
if (day > DaysInMonth()) {
    day = 1;
    month++;
}
if (month >= 12) {
    year++
}

[viiv]

[uquote="WiseLord",url="/forum/viewtopic.php?p=3220452#p3220452"]А как бы такую логику делал я:

Спойлер

Код: Выделить всё

// Функция определяет число дней в месяце, уже с учётом високосных годов
static uint8_t DaysInMonth(void)
{
	uint8_t ret = month;
	if (ret == 2) {
		ret = year & 0x03;
		ret = (ret ? 29 : 28);
	} else {
		if (ret > 7)
			ret++;
		ret |= 30;
	}
	return ret;
}
}

[/uquote]

1)

Код: Выделить всё

ret = (ret ? 29 : 28);

29<==>28 помянять бы местами
2) 2100 (2200) году будет неверно работать, но это уже мелочи.
3) чтобы понять, надо напрячься комментарий бы добавить к строке " ret |= 30;"
завести массив кол-ва дней в месяцах (во флеш памяти) и индексироваться - просто, понятно с первого взгляда, и займен не намного больше места

[Reflector]

[uquote="viiv",url="/forum/viewtopic.php?p=3220521#p3220521"]завести массив кол-ва дней в месяцах (во флеш памяти) и индексироваться - просто, понятно с первого взгляда, и займен не намного больше места[/uquote]
Если оптимизировать вариант WiseLorda, например, так:

Код: Выделить всё

uint8_t DaysInMonth()
{
	if (month == 2)  return 28 + !(year & 3);
	return month + (month >> 3) | 30;
}

то разница с функцией на основе таблицы получается 24 байта для STM32F4.
А хотя нет, похоже это из-за того, что я функцию в цикле вызывал, без него 12 байт.

[arkhnchul]

это уже компактность ради компактности, а не читаемости

[WiseLord]

29<==>28 помянять бы местами

Мда... и ошибка уже старая...

Как говорится, срочно в номер: проект раз, проект два, проект три

[Аlex]

[uquote="Reflector",url="/forum/viewtopic.php?p=3220534#p3220534"]Если оптимизировать вариант WiseLorda, например, так:

Код: Выделить всё

uint8_t DaysInMonth()
{
	if (month == 2)  return 28 + !(year & 3);
	return month + (month >> 3) | 30;
}

[/uquote]
Тогда уж можно вообще одной строкой написать :

Код: Выделить всё

return (month == 2) ? (28 + !(year & 3)) : (month + (month >> 3) | 30);

[viiv]

[uquote="WiseLord",url="/forum/viewtopic.php?p=3220589#p3220589"]Мда... и ошибка уже старая...[/uquote]

Выносить определение високосности в отдельную функцию (по желанию inline) и всегда (во всех проектах) использовать ее

Спойлер

Код: Выделить всё

/*
 * високосный ли год
 *   year (c 2000-го года): 0=2000, 1=2001, ...
 *
 * c 2000 по 2255 годы: 2100 и 2200 не являются високосными
 */
__INLINE__ bool_t is_leap_year (uint8_t year)
{
        if (year & 0x03)
                return 0;

#ifdef LEAP_YEAR_EXEPTIONS  /* съэкономим несколько байт, если не проверяем исключения (годы: 2100 и 2200)*/
        if (year == 100 || year == 200)
                return 0;
#endif

        return 1;
}

Тогда Ваш код еще понятней станет (не очивидные с первого взляда места я бы откомментировал):

Спойлер

Код: Выделить всё

uint8_t DaysInMonth (uint8_t month, uint8_t year)
{
   if (month == 2)
        return is_leap_year (year) ? 29 : 28;

   if (month > 7)            /* длинные и короткие месяцы чередуются, исключение: */
      month++;               /*  после длинного июля идет длинный август  */

   return month |= 30;       /* используется только младший бит month */
}

[ra9ust]

Ну вот, вчерашнее сравнение настроек переменных с еепром ужалось вот до такой функции.

[Аlex]

А зачем фигурные скобки ужимать в кучу ? Смотрится отвратительно.

Добавлено after 3 minutes 48 seconds:
Что уже не так написали :

Код: Выделить всё

void contr_eep (void)
{   
for(comp=0;comp<66;comp++){ if(mass[comp]!=eep_mass[comp])result=0;}  if(result==1){delayt=0;while(delayt<200){lcd_gotoxy(0,0);lcd_putsf("  TEST EEPROM         OK!       ");}w=0;}else{lcd_gotoxy(0,0);lcd_putsf("  TEST EEPROM       FAILURE!    ");if(eeppp&0b0011111110010000){PORTB.5=0;}else{PORTB.5=1;}if(PINC.3==0){PORTB.5=0;dreb_0=6;w=0;}} //пищит каждые примерно 2 минуты
}

?
Всё в одной строке

[ra9ust]

Да я уже как-то привык про скобки. Про что уже не так написал , что-то не понял.

[viiv]

ra9ust, структурируйте код.

Хотим сохранять конфигурацию в EEPROM. Отлично. Зачем? Чтобы при сбросе/включении начать работать с сохраненной конфигурацией.
1) Пользователю не интересно видеть сообщение "TEST EEPROM OK! " Зачем оно? Если при включении самотестирование проходит - молча начинаем работать. Представьте, включаете Вы телевизор, а там:

Код: Выделить всё

Тestng flash memory...Ok
Testing SRAM...Ok
Testing EEPROM...Ok
Testing power supply voltages...Ok
....

2) У вас есть конфигурация устройства. Так? Не вижу причин не зафиксировать это средствами языка программирования (объявить соотвктствующую структуру):

Код: Выделить всё

typedef struct _device_config_t {
   // здесь все параметры конфигурации
} device_config_t;

3) mass[] - конфигурация в SRAM eep_mass[] - конфигурация в EEPROM. Так?
Непонятно, зачем их сравнивать. mass откуда берется? Читается из EEPROM?
Т.е. Вы прочитали EEPROM, работаете-работаете, а затем обнаруживаете,
что EEPROM (или конфигурация в SRAM) странным образом изменилась.

При старте устройства желательно бы проверить целостность конфигурации в EEPROM (например, питание устройства может быть отключено в момент записи конфигурации), выбирайте:
a) в EEPROM хранить контрольную сумму
б) в EEPROM держать несколько копий
в) комбинация двух методов
г) что-то еще

3) Раз есть конфигурация, то нужны:
a) функция сброса параметров конфигурации в значения по умолчанию. Т.е. если обнаружено, что в EEPROM лежит каая-то фигня, то надо привести
конфигурацию в заведомо правильное состояние (значения по умолчанию, заводские установки - называйте как хотите)

Код: Выделить всё

 void device_default_config (device_config_t* c) {
   // устанавливаем значения по умолчанию
}

б) функцию загрузки конфигурационных параметров:

Спойлер

Код: Выделить всё

/* например, есть функции чтение байта из EEPROM по указанному адресу */
uint8_t eeprom_read_byte (unsigned addr);
/* мы хотим контолировать целестность сохраненной конфигурации посредсвом контрольной суммы */
uint16_t cksum_update (uint16_t cksum, uint8_t);


/* возвращает 0, если в EEPROM лежит плохая конфигурация, и значения конфигурационных
   параметров установлены в начальное состояние */
bool_t device_load_config (device_config_t* c) {
    uint8_t data;
    uint16_t cksum = CKSUM_INIT_VALUE; /* начальное значение контрольной суммы */
    uint16 addr = DEVICE_CONFIG_ADDR; /* начиная с DEVICE_CONFIG_ADDR лежит конфигурация устройства */
    uint8_t *p = (uint8_t *) (c);
    uint8_t *limit = p + sizeof (*c);

    while (p < limit) {
         data = eeprom_read_byte (addr++);
         cksum = cksum_update (cksum, data);
         *p++ = data;
    }
    /* читаем дополнительно байты контрольной суммы */
    cksum = cksum_update (cksum, eeprom_read_byte (addr++));
    cksum = cksum_update (cksum, eeprom_read_byte (addr));
    if (cksum != CKSUM_GOOD) {    /* контрольная сумма плохая */
        device_default_config (c);         /* сбрасываем конфигурацию */
        return 0;
    }
    
    return 1;
};

функция device_load_config () будет нормально работать во ВСЕХ ваших проектах,
достаточно чтобы была определена device_config_t, функции чтения EEPROM, функции контрольной суммы.

в) функцию сохранения конфигурационных параметров.
г) ...

[ra9ust]

Ну так это всё у меня и сделано, естественно так как мне удобно.