Форум разработчиков электроники ELECTRONIX.ru

mdmitry

Sep 26 2013, 15:41

С какой целью в usrlib.h в описании шаблона класса ring_buffer

Код

template<typename T, uint16_t Size, typename S = uint8_t>
class ring_buffer

тип S uint8_t? На мой взгляд заложена потенциальная опасность при использовании умолчания шаблона.
Код:

CODE

#include <iostream>
#include "usrlib.h"
using namespace std;

#define RFBUF_SIZE 	1024
#define DTBUF_SIZE	255

usr::ring_buffer<uint8_t, RFBUF_SIZE, uint16_t> full_description;
usr::ring_buffer<uint8_t, RFBUF_SIZE> short_description;

uint8_t databuf[DTBUF_SIZE];

int main()
{
	for(uint8_t i=0; i < DTBUF_SIZE; i++)
		databuf[i] = i;

	full_description.write(&databuf[0], DTBUF_SIZE - 8);
	short_description.write(&databuf[0], DTBUF_SIZE - 8);

	cout << dec;
	cout << "full_description: get_count = " << full_description.get_count() << " get_free_size = " << full_description.get_free_size() << endl;
	cout << "short_description: get_count = " << short_description.get_count() << " get_free_size = " << short_description.get_free_size() << endl;

	full_description.write(&databuf[0], DTBUF_SIZE - 8);
	short_description.write(&databuf[0], DTBUF_SIZE - 8);

	cout << "full_description: get_count = " << full_description.get_count() << " get_free_size = " << full_description.get_free_size() << endl;
	cout << "short_description: get_count = " << short_description.get_count() << " get_free_size = " << short_description.get_free_size() << endl;

	return 0;
}

Вывод:

Код

full_description: get_count = 247 get_free_size = 777
short_description: get_count = ч get_free_size =
full_description: get_count = 494 get_free_size = 530
short_description: get_count = о get_free_size =

При включенных предупреждениях компилятора потенциальная опасность видна сразу.
Вопрос: из каких соображений в шаблоне по умолчанию тип S не совпадает с типом Size?
Спасибо.

dxp

Sep 27 2013, 07:53

QUOTE (mdmitry @ Sep 26 2013, 22:41)

Вопрос: из каких соображений в шаблоне по умолчанию тип S не совпадает с типом Size?

Ноги растут из "древности", из первых версий, когда основными процессорами были AVR и MSP430, актуально было экономить каждый байт, а буферов таких размеров и быть не могло - там памяти столько не было. Сегодня, наверное, самое правильное замутить тут traits. Подумаем на эту тему. Спасибо!

mdmitry

Sep 27 2013, 15:57

В функции

Код

template<typename T, uint16_t Size, typename S>
T usr::ring_buffer<T, Size, S>::pop_item_back()
{

    if(Last == 0)
        Last = Size - 1;
    else
        --Last;

    Count--;
    return Buf[Last];;
}

наверное, не требуется в последней строке (где return) два символа ;? Это в версии из trunk.

Gav

Jul 26 2018, 12:08

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

А вопрос у меня такой.

версия userlib.h: Version: v5.1.0

почему методы <write> и <read>, класса <ring_buffer> такие не единообразные?

Код

bool usr::ring_buffer<T, Size, S>::write(const T* data, const S cnt)
{
    if( cnt > (Size - Count) )
        return false;

    for(S i = 0; i < cnt; i++)
        push_item(*(data++));

    return true;
}
void usr::ring_buffer<T, Size, S>::read(T* data, const S cnt)
{
    S nItems = cnt <= Count ? cnt : Count;

    for(S i = 0; i < nItems; i++)
        data[i] = pop_item();
}

т.е. что я имею ввиду:

<read>: контролирует кол-во данных и если их меньше, чем запрашивают, то считывает столько сколько есть, однако, при этом не возвращает кол-во считанных данных.
<write>: ничего не делает, если кол-во свободного места меньше, чем хотят записать, а не записывает столько сколько может (аналогично read), с последующим возвратом кол-во записанных данных.

Мне кажется, что более качественно было бы, если read считывал MIN(Count, cnt) и возвращал бы это кол-во данных, а write аналогично, писал бы MIN(Size-Count, cnt) и возвращал бы кол-во записанных данных.
Ведь тут <Count> - это не размер в байтах, а именно кол-во единиц элементов типа <T>.

Используется данный класс <ring_buffer> в компоненте <channel>, где тоже странная реализация у методов:

Код

S OS::channel<T, Size, S>::write_isr(const T* data, const S count)
{
    TCritSect cs;

    const S free = pool.get_free_size();
    S qty = free < count ? free : count;
    pool.write(data, qty);
    resume_all_isr(ConsumersProcessMap);
    return qty;
}
S OS::channel<T, Size, S>::read_isr(T* const data, const S max_size)
{
    TCritSect cs;

    const S avail = pool.get_count();
    S count = avail < max_size ? avail : max_size;
    pool.read(data, count);
    resume_all_isr(ProducersProcessMap);
    return count;
}

т.е. метод write_isr пользуется методом <pool.write>, но не контролирует возвращаемый им признак.
А read_isr, зачем-то контролирует <pool.Count>, обрезает считываемый размер и при этом не смотрит на возращаемое значение pool.read, когда сам метод <ring_buffer.read> делает все тоже самое.. и по делу.

Куда компактней было бы написать так
(в случае, если бы методы ring_buffer: read/write были бы универсальными):

Код

S OS::channel<T, Size, S>::write_isr(const T* data, const S count)
{
    TCritSect cs;

    S qty = pool.write(data, count);
    resume_all_isr(ConsumersProcessMap);
    return qty;
}
S OS::channel<T, Size, S>::read_isr(T* const data, const S max_size)
{
    TCritSect cs;

    S count = pool.read(data, max_size);
    resume_all_isr(ProducersProcessMap);
    return count;
}

Раз компонент <channel> использует свой (под себя написанный) класс <ring_buffer>, то и перезакладываться на контроль чужого (для класса channel) <Count> при вызове read/write нет никакой необходимости.

Я не прав?

Сергей Борщ

Jul 28 2018, 07:00

QUOTE (Gav @ Jul 26 2018, 15:08)

Мне кажется, что более качественно было бы, если read считывал MIN(Count, cnt) и возвращал бы это кол-во данных, а write аналогично, писал бы MIN(Size-Count, cnt) и возвращал бы кол-во записанных данных.

Ваше предложение разумное. Я сам буквально пару недель назад пришел к такому же выводу. Но мы не можем радикально менять поведение OS::channel для совместимости со старыми проектами. В то же время ничего не мешает нам написать новый класс. Давайте все вместе придумаем ему понятное название и вместе напишем реализацию. Мне пока кроме stream и new_channel ничего в голову не приходит. C new_channel закладываем себе грабли - через сколько-то лет надо будет убирать слово new_ и заменять на что-то другое. advanced_channel?

dxp

Jul 30 2018, 01:53

Цитата(Gav @ Jul 26 2018, 19:08)

почему методы <write> и <read>, класса <ring_buffer> такие не единообразные?

Код

bool usr::ring_buffer<T, Size, S>::write(const T* data, const S cnt)
{
    if( cnt > (Size - Count) )
        return false;

    for(S i = 0; i < cnt; i++)
        push_item(*(data++));

    return true;
}
void usr::ring_buffer<T, Size, S>::read(T* data, const S cnt)
{
    S nItems = cnt <= Count ? cnt : Count;

    for(S i = 0; i < nItems; i++)
        data[i] = pop_item();
}

т.е. что я имею ввиду:

<read>: контролирует кол-во данных и если их меньше, чем запрашивают, то считывает столько сколько есть, однако, при этом не возвращает кол-во считанных данных.
<write>: ничего не делает, если кол-во свободного места меньше, чем хотят записать, а не записывает столько сколько может (аналогично read), с последующим возвратом кол-во записанных данных.

Такая асимметрия возникает из-за характера применения функций чтения и записи. При чтении ожидающий процесс ничего не знает о количестве данных в буфере, если специально не запрашивать. Поэтому как только что-то там есть, это что-то отдаётся "читателю". У "писателя" ситуация несколько иная - как правило при записи пишется некий законченный блок данных (пакет) и нет никакого смысла дробить его из-за появления в буфере сколько-нибудь места. Если делать симметрично, то некоторых случаях будет возникать либо торможение на стороне приёмника (если отдавать только запрошенное количество данных), либо будет возникать дополнительная обязательная нагрузка на пользовательский код - вместо того, чтобы один раз вызвать запрос за запись в канал (а класс буфера там спроектирован именно как основа для сервисного класса ОС channel), пользователю придётся отслеживать порции записи и в цикле дописывать.

Вот такие соображения. Тут можно спорить, что лучше, но вот такие рассуждения были 15 лет назад, исходя их тех реалий (в первую очередь возможностей элементной базы и обозримого класса задач).

Цитата(Gav @ Jul 26 2018, 19:08)

Мне кажется, что более качественно было бы, если read считывал MIN(Count, cnt) и возвращал бы это кол-во данных, а write аналогично, писал бы MIN(Size-Count, cnt) и возвращал бы кол-во записанных данных.
Ведь тут <Count> - это не размер в байтах, а именно кол-во единиц элементов типа <T>.

См. выше.

Цитата(Gav @ Jul 26 2018, 19:08)

Используется данный класс <ring_buffer> в компоненте <channel>, где тоже странная реализация у методов:

Код

S OS::channel<T, Size, S>::write_isr(const T* data, const S count)
{
    TCritSect cs;

    const S free = pool.get_free_size();
    S qty = free < count ? free : count;
    pool.write(data, qty);
    resume_all_isr(ConsumersProcessMap);
    return qty;
}
S OS::channel<T, Size, S>::read_isr(T* const data, const S max_size)
{
    TCritSect cs;

    const S avail = pool.get_count();
    S count = avail < max_size ? avail : max_size;
    pool.read(data, count);
    resume_all_isr(ProducersProcessMap);
    return count;
}

т.е. метод write_isr пользуется методом <pool.write>, но не контролирует возвращаемый им признак.
А read_isr, зачем-то контролирует <pool.Count>, обрезает считываемый размер и при этом не смотрит на возращаемое значение pool.read, когда сам метод <ring_buffer.read> делает все тоже самое.. и по делу.

Использование сервиса channel в обработчиках прерываний в общем случае небезопасно, т.к. в случае нехватки ресурса будет произведена попытка встать на ожидание (ресурса) и отдать управление ядру, чего в прерывании делать нельзя. Поэтому позже (кстати, совсем не так давно) были добавлены функции *_isr, которые не приводят к фатальным последствиям, т.к. проверяют наличие ресурса и только после этого обращаются к нему. По нюнасам реализации прокомментировать не могу, автор этого добавления не я.

Можно ли тут что-то отрулить, не берусь с ходу судить: с одной изменения могут сломать что-то у пользователя, с другой - те, что были внесены совсем недавно (read/write_isr) вряд ли "обросли историей" и если логика использования не меняется, то реализацию можно и подшаманить безопасно.

Цитата(Сергей Борщ @ Jul 28 2018, 14:00)

Ваше предложение разумное. Я сам буквально пару недель назад пришел к такому же выводу. Но мы не можем радикально менять поведение OS::channel для совместимости со старыми проектами. В то же время ничего не мешает нам написать новый класс. Давайте все вместе придумаем ему понятное название и вместе напишем реализацию. Мне пока кроме stream и new_channel ничего в голову не приходит. C new_channel закладываем себе грабли - через сколько-то лет надо будет убирать слово new_ и заменять на что-то другое. advanced_channel?

Да, трогать старые channel::read/write, полагаю, нельзя - обязательно сломается пользовательский код. И если хочется своего, то таки да, такая возможность есть штатно - написать свою реализацию в виде расширения. По названию могу предложить - pipe. Смысл и уровень абстракции такой же как и у channel в данном контексте, а имя совершенно другое и даже короче.