Кодревью master 88db70ede8 #3

New Issue

mysol93 · 2026-04-08T18:09:24+03:00

mysol93 commented

2026-04-08 18:09:24 +03:00

в мастере вижу ряд проблем, которые лучше исправить.

Старайтесь не использовать константы напрямую, т.е. так называемые magic numbers, когда нужно будет вернуться к коду поосле перерыва вы будете долго вспоминать откуда они взялись. Вводи где возможно parameters, где невозможно - localparam. Пример из файла generator.sv

		вот это неоч
		   |
           |
           V
    input [31:0]           pulse_width,
    input [31:0]           pulse_period,

Сделайте 
.
.
.
	parameter int PULSE_WIDTH_MAX = 100000
	parameter int PULSE_PERIOD_MAX = 100000)
(
.
.
.
    input wire [$clog2(PULSE_WIDTH_MAX)-1:0]   pulse_width,
    input wire [$clog2(PULSE_PERIOD_MAX)-1:0]  pulse_period,

В этом же файле

wire start_pulse = start & ~start_d; <=== фу, давайте всё присваивать в always/initial блоке или assign

В этом же файле cnt_pulse_num и cnt_period изменяют свои значения в разных if-statements. Ниоч хорошо, можно легко сломать логику отправив 2 старта подряд. Давайте так не делать.

К файлу sampler.sv Тоже есть вопрос вот тут

if (data_in == 12'b100000000000) begin <=== тут правильное условие для data_in?

По тестам. Прошу увеличивать покрытие тестов путём генерации случайных чисел для воздействия на входы. Увеличивать их количество, количество итераций, а так же необходимо сравнение выходов модулей и проверка этих выходов. Т.е., например, если мы на выходе DUT получаем данныые, то мы должны сгененрировать референсные данные и сравнить результаты. В случае совпадения сделать вывод о верности теста в стиле PASS/FAIL. Тесты должны быть автоматические, т.е. запустил - получил результат. В случае отрицательного результата нужно иметь возможность изучить отладочную информацию (вейвформы, логи и т.д.)
Зайлете скрипты запуска тестов. Список файлов для тестов можете внести в файл-листы.

[в мастере](https://git.radiophotonics.ru/baulin.fa/reflectometer_fpga_project/commit/88db70ede84b169ef9bfe4e9d41290dbe1b876ec) вижу ряд проблем, которые лучше исправить. 1) Старайтесь не использовать константы напрямую, т.е. так называемые magic numbers, когда нужно будет вернуться к коду поосле перерыва вы будете долго вспоминать откуда они взялись. Вводи где возможно parameters, где невозможно - localparam. Пример из файла [generator.sv](https://git.radiophotonics.ru/baulin.fa/reflectometer_fpga_project/commit/88db70ede84b169ef9bfe4e9d41290dbe1b876ec#diff-49ad991c18d616ce195a17c128450e5f7a949f86) ``` вот это неоч | | V input [31:0] pulse_width, input [31:0] pulse_period, Сделайте . . . parameter int PULSE_WIDTH_MAX = 100000 parameter int PULSE_PERIOD_MAX = 100000) ( . . . input wire [$clog2(PULSE_WIDTH_MAX)-1:0] pulse_width, input wire [$clog2(PULSE_PERIOD_MAX)-1:0] pulse_period, ``` В этом же файле ``` wire start_pulse = start & ~start_d; <=== фу, давайте всё присваивать в always/initial блоке или assign ``` В этом же файле **cnt_pulse_num** и **cnt_period** изменяют свои значения в разных if-statements. Ниоч хорошо, можно легко сломать логику отправив 2 старта подряд. Давайте так не делать. К файлу [sampler.sv](https://git.radiophotonics.ru/baulin.fa/reflectometer_fpga_project/commit/ad6d6a4e2b1550e715e56b6a33e00eded1630e73#diff-ec68d2f12e960ca117c4ea1287cbbf0808e314ba) Тоже есть вопрос вот тут ``` if (data_in == 12'b100000000000) begin <=== тут правильное условие для data_in? ``` 2) По тестам. Прошу увеличивать покрытие тестов путём генерации случайных чисел для воздействия на входы. Увеличивать их количество, количество итераций, а так же необходимо сравнение выходов модулей и проверка этих выходов. Т.е., например, если мы на выходе DUT получаем данныые, то мы должны сгененрировать референсные данные и сравнить результаты. В случае совпадения сделать вывод о верности теста в стиле PASS/FAIL. Тесты должны быть автоматические, т.е. запустил - получил результат. В случае отрицательного результата нужно иметь возможность изучить отладочную информацию (вейвформы, логи и т.д.) 3) Зайлете скрипты запуска тестов. Список файлов для тестов можете внести в файл-листы.

awe commented

2026-04-21 19:54:53 +03:00

(╯°□°)╯︵ ┻━┻

😆 1

ilianova.ds commented

2026-04-30 12:07:07 +03:00

"Cтарайтесь не использовать константы напрямую, т.е. так называемые magic numbers..."- ширина pulse_width и pulse_period была выбрана с запасом исходя из возможного максимального значения этих параметров, установленных и согласованных в тз, вряд ли она будет меняться, это не magic number, не вижу смысла в его парметризации.
"В этом же файле cnt_pulse_num и cnt_period изменяют свои значения в разных if-statements. Ниоч хорошо, можно легко сломать логику отправив 2 старта подряд."- защита от дурака реализована в блоке контроллера. Отправки двух стартов подряд быть не может.
поправила условие на параметрическое:
if (data_in == {1'b1, {(DATA_WIDTH-1){1'b0}}})
тесты для сэмплера, контроллера и аккумулятора поправлены в соответствии с требованиями, тест генератора - в разработке.

1) "Cтарайтесь не использовать константы напрямую, т.е. так называемые magic numbers..."- ширина pulse_width и pulse_period была выбрана с запасом исходя из возможного максимального значения этих параметров, установленных и согласованных в тз, вряд ли она будет меняться, это не magic number, не вижу смысла в его парметризации. 2) "В этом же файле cnt_pulse_num и cnt_period изменяют свои значения в разных if-statements. Ниоч хорошо, можно легко сломать логику отправив 2 старта подряд."- защита от дурака реализована в блоке контроллера. Отправки двух стартов подряд быть не может. 3) поправила условие на параметрическое: `if (data_in == {1'b1, {(DATA_WIDTH-1){1'b0}}})` 4) тесты для сэмплера, контроллера и аккумулятора поправлены в соответствии с требованиями, тест генератора - в разработке.

ilianova.ds closed this issue

2026-05-12 13:49:02 +03:00

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: baulin.fa/reflectometer_fpga_project#3

Кодревью master 88db70ede8 #3

Кодревью master `88db70ede8` #3