fix (tested on PCU, LV-BMS, HV-BMS and LCU)#594
fix (tested on PCU, LV-BMS, HV-BMS and LCU)#594victor-Lopez25 wants to merge 17 commits intodevelopmentfrom
Conversation
| Task& task = tasks_[bit_index]; | ||
| task.callback(); | ||
|
|
||
| SchedLock(); |
There was a problem hiding this comment.
No me mola mucho la verdad que el bitmap se limpie despues del callback, si hay problemas de rendimiento y el timer vuelve a hacerle set a la tarea la segunda ejecucion no se ejecuta, por lo demas no veo muchos mas problemas a hacerle clear despues del callback, se podria quedar asi pero solo digo
There was a problem hiding this comment.
Era por poner un solo lock, si lo ves mejor poner dos locks y así poder hacerlo antes lo puedo cambiar
|
|
||
| uint64_t Scheduler::get_global_tick() { | ||
| SchedLock(); | ||
| uint64_t val = global_tick_us_ + Scheduler_global_timer->CNT; |
There was a problem hiding this comment.
No entiendo porque esto necesita un lock, la lectura del CNT es atomica es imposible que esto se corrompa
There was a problem hiding this comment.
la lectura del CNT sí pero la lectura de global_tick_us_ no pq es uint64_t
| } | ||
| pop_front(); | ||
|
|
||
| SchedLock(); |
There was a problem hiding this comment.
tampoco tiene sentido hacerle disable al timer dentro del interrupt del timer
There was a problem hiding this comment.
Tengo que probar si realmente no hace falta, pq es una de las primeras cosas que probé ayer en las placas y la dejé
There was a problem hiding this comment.
Según la prueba de Boris en la LCU no hace falta pero voy a probarlo mañana (?) en las otras placas también
| return; | ||
| } | ||
|
|
||
| SchedLock(); |
There was a problem hiding this comment.
esto esta dentro del interrupt del timer no tiene sentido hacer disable al timer
| if (Scheduler_global_timer->CNT > current_interval_us_) [[unlikely]] { | ||
| uint32_t offset = Scheduler_global_timer->CNT - current_interval_us_; | ||
| Scheduler_global_timer->CNT = 0; | ||
| global_tick_us_ += offset; |
There was a problem hiding this comment.
esto esta simplemente mal, te estas adelantando en el tiempo, yo me guardaria otro valor que fuera accumulated offset y cuando esto pasa lo incrementas en la linea 307
There was a problem hiding this comment.
Lo he cambiado para que sume:
uint32_t prevCnt = Scheduler_global_timer->CNT;
Scheduler_global_timer->CNT = 0;
global_tick_us_ += prevCnt;Pero cuando lo probamos para ver qué mandaba parecía que subía un poco demasiado rápido (por poco)
Puede que sea también por el get_global_tick() que es lo que usaba para ver qué tiempo tenía el scheduler
Vimos dos cosas:
- Empezaba en un valor muy alto, como si hubiese pasado ya un tiempo
- Subía demasiado rápido, no llegaba a duplicar, igual 1.2x o por ahí al tiempo real (medido a ojo entre Joan y yo)
ST-LIB Release Plan
Pending changes
|
victor-Lopez25
changed the title
victor-Lopez25
force-pushed
the
fix/scheduler-race-cond-part2
branch
from
dfd47d2 to
d6af843
Compare
jorgesg82
force-pushed
the
fix/scheduler-race-cond-part2
branch
from
a17d0eb to
f4a12ef
Compare
jorgesg82
force-pushed
the
fix/scheduler-race-cond-part2
branch
from
c0d15a7 to
581bb19
Compare
3 participants
release: patch
summary: fix remaining scheduler race conditions