Skip to content

Addition of ACK functionality#6

Open
kaszczech wants to merge 3 commits into
mainfrom
ack
Open

Addition of ACK functionality#6
kaszczech wants to merge 3 commits into
mainfrom
ack

Conversation

@kaszczech

Copy link
Copy Markdown
Contributor

No description provided.

@m-wojnar m-wojnar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kilka uwag ogólnych:

  1. Gdy tworzysz PR, dodaj mnie jako recenzenta - wówczas dostanę powiadomienie i będę wiedział, że jest coś do sprawdzenia. Dodatkowo, tera jest możliwość, aby dodać do recenzji GitHub copilot i wtedy LLM sam zrobi code review - też warto, szczególnie na sam początek.
  2. Gdy widzisz, że testy nie przeszły (czyli CI się wyrzuciło, jest czerwony krzyżyk na PR), zawsze trzeba doprowadzić kod do działania. W tym przypadku to nic strasznego, bo to tylko linter daje uwagi, ale uwaga na przyszłość.
  3. I to najważniejsze - z tego co widzę, w ogóle nie jest zaimplementowana opóźniona nagroda po wysłaniu ACK. Przydałaby się taka opcja (najlepiej żeby można to było skonfigurować), bo w tej chwili wysyłanie ACK jest opcjonalne, a dodatkowo nieopłacalne - przecież można zignorować wysłanie ACK, a zamiast tego robić cały czas TX i będzie za to większa nagroda. Więc dobrze byłoby, aby nagroda za wysłany pakiet była przyznana tylko wtedy, gdy druga stacja odeśle ACK (a jeśli ona nie odeśle, to powinna dostać karę, a nadawca zero). Oczywiście w jakimś zakresie czasu, więc można to spokojnie monitorować z użyciem transmission_history, a nagrody do przyznania można zapisać gdzieś w stanie symulatora

Comment thread ltc/run.py
Comment thread ltc/run.py Outdated
Comment thread ltc/sim/constants.py Outdated
Comment thread ltc/sim/constants.py Outdated
Comment thread ltc/sim/constants.py Outdated
Comment thread ltc/sim/sim.py Outdated
Comment thread ltc/sim/sim.py Outdated
Comment thread ltc/sim/sim.py Outdated
Comment thread ltc/sim/sim.py Outdated
Comment thread test/sim/test_sim.py Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants