Автор: 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 в данном контексте, а имя совершенно другое и даже короче.