Efuse Refactoring #1749
Efuse Refactoring #1749
Conversation
Aditya-Dhiman4
force-pushed
the
amir/loadswitch_refactoring
branch
from
db6923d to
8767da1
Compare
Lucien950
added
Systems
Aditya-Dhiman4
force-pushed
the
amir/loadswitch_refactoring
branch
from
cecab5b to
8f6e10e
Compare
Aditya-Dhiman4
force-pushed
the
amir/loadswitch_refactoring
branch
from
8f6e10e to
7f7ebcd
Compare
|
|
||
| // Setting faults for st_vnd5 efuse | ||
| // TODO: Verify these | ||
| if (fault_table_idx == L_L) |
There was a problem hiding this comment.
case statement this
| * https://octopart.com/datasheet/vnd5t100ajtr-e-stmicroelectronics-21218840 | ||
| */ | ||
|
|
||
| #define NOMINAL_V 3.0f |
There was a problem hiding this comment.
This needs to be characterized because nominal input voltage is usually 3.3
There was a problem hiding this comment.
unless data sheet said otherwise
There was a problem hiding this comment.
I thought I saw this in the datasheet but I didnt so ill set this to 3.3
There was a problem hiding this comment.
Oh actually this I might have been also referring to the power manager, if you look at the removed code from the power_manager.c file we compare with 3.0
There was a problem hiding this comment.
lets keep at 3.3 but this needs to be characterized
firmware/shared/src/io/io_efuse/io_efuse_ST_VND5/io_efuse_ST_VND5.c
Outdated
Show resolved
Hide resolved
| } | ||
| else | ||
| { | ||
| efuse->ti_tps28->faults.raw = 0x03; |
There was a problem hiding this comment.
what is 0x03 supposed to mean here
There was a problem hiding this comment.
that is not correct lol, basically if the diagnostics pin is down, the fault pin still indicates for overcurrent and thermal shut down, but I am unsure if I am supposed to check for those faults in the same way if the diagnostics is high or not. They only have a truth table for fault checking if the diagnostics pin is high.
There was a problem hiding this comment.
Surely if the diag pin is not high we shouldn't do any checks. Also isn't diag pin an input to the chip?
There was a problem hiding this comment.
Yeah its an input i just forgot to make a setter for it. The reason why I still want to perform checks is because the datasheet says if diagnostics are disabled the fault pin will still report overcurrent and thermal shutdown, I need to discuss this a bit more with you
There was a problem hiding this comment.
Lowkey these can just be done in the efuse.hpp file they are very short functions so allocating an entire file to them seem unecessary
| static constexpr uint8_t H_L = 0x02U; | ||
| static constexpr uint8_t H_H = 0x03U; | ||
|
|
||
| ST_VND5_Efuse::ST_VND5_Efuse( |
There was a problem hiding this comment.
please do this in the hpp file
There was a problem hiding this comment.
other than it being kinda short I dont fully get why we should define the functions in the header files only, like whats the advantage here
There was a problem hiding this comment.
well it is a constructor function so convention-wise it should be part of the .hpp file so if you look at hw_pwminput you can see that is bundled up. I think @Lucien950 can prob give the c++ optimization answer but in my view at least the cpp function should only have the functions associated with the class, and the constructor should be in the hpp
There was a problem hiding this comment.
Also please consteval the constructor
There was a problem hiding this comment.
also i think if you leave it in the hpp file it can implicitly inline the constructor vs here if it is in the cpp you cant inline nomore
There was a problem hiding this comment.
wait just to be clear you want me to just move the constructor to the header or the entire file? I thought you wanted me to move it to the header?
There was a problem hiding this comment.
the constructor
|
|
||
| namespace io | ||
| { | ||
| TI_TPS25_Efuse::TI_TPS25_Efuse(const hw::Gpio &enable_gpio, const hw::Adc &sns_adc_channel, const hw::Gpio &pgood) |
There was a problem hiding this comment.
same as before please do this in the hpp file
| hw_gpio_writePin(efuse->enable_gpio, false); | ||
| } | ||
|
|
||
| static bool io_TI_TPS25_Efuse_pgood(const Efuse *efuse) |
There was a problem hiding this comment.
there is an important consideration to note that when checking pgood this has to be when there is a voltage on the VCC or something like and we can just check this. Double check this with the datasheet to when we are allowed to check pgood
There was a problem hiding this comment.
please change back
There was a problem hiding this comment.
Also please fix the tests
Changelist
Testing Done
Resolved Tickets