ds2430 eeprom

[mvfpoa]

I created the device and the system I was interfacing I created one more function to read rom_id when device is alone in the bus I think the system merge will be postponed, since I have made it with HAL, no LL

[mvfpoa]

I think I corrected it all. Sorry if I left something behind. Have you seen my stm32 with HAL? What do you think about that? It is weird to read hal code at a low level standard file. May be we could leave this file to a future pull.

[MaJerle]

There are things to be changed:

• Missing curly brackets in some if statements
• Wrong spacing in some if statements in HAL low-level driver
• Every variable can be initialize ONLY at the beginning of the block, not in the middle
• Do not cast void * as it is automatically casted to another pointer type
• OW_ASSERT0 is wrongly used in some cases. Cannot be used if function returns owr_t type
• Avoid using too many return statements as in case of read_rom function in ow.c
• Curly brackets must be in same line, not in new line (even for function declaration)
• Always use size_t parameter for type indicating length or size.

Use this link to see how it looks like: https://github.com/MaJerle/onewire_uart/pull/1/files

Sorry, there is some work to be done. This is the only way to maintain the code properly.

[mvfpoa]

I swear I looked most of this items. :-( I created regexs to check them... I will review it again.

[mvfpoa]

Hi again. Could you please indicate the place where you found this topic: "OW_ASSERT0 is wrongly used in some cases. Cannot be used if function returns owr_t type" I Cannot find major explanations about good practices around function returns around this topic: "Avoid using too many return statements as in case of read_rom function in ow.c". I changed the function you indicated, so we can start discussing what would be acceptable. I generally choose size_t in place of int or uint_t. I have checked the code and the cases I supposed you were talking about seems correct to me. Maybe could you also indicate what you have found?

Thanks.

[MaJerle]

Could you please indicate the place where you found this topic: "OW_ASSERT0 is wrongly used in some cases. Cannot be used if function returns owr_t type"

Yes, no problem. Function ow_ds2430_storage_write_scratchpad_raw returns owr_t type, and should use OW_ASSERT(), not OW_ASSERT0() as it is now. Same goes for function ow_ds2430_storage_write_scratchpad and some others. Didn't check all cases.

I Cannot find major explanations about good practices around function returns around this topic: "Avoid using too many return statements as in case of read_rom function in ow.c". I changed the function you indicated, so we can start discussing what would be acceptable.

Your implementation of function ow_read_rom_raw could be written with less return statements. See how it is implemented for DS18X20 driver. Nested if statements.

https://github.com/MaJerle/onewire_uart/blob/66865b3e0d9ce0e872025f7064f99caa36cf227a/onewire_uart/src/devices/ow_device_ds18x20.c#L86

I generally choose size_t in place of int or uint_t. I have checked the code and the cases I supposed you were talking about seems correct to me. Maybe could you also indicate what you have found?

Function ow_ds2430_storage_write_scratchpad has length parameter. I would do 2 changes:

• Change type to size_t
• Change name to btw meaning bytes to write. Similar for read, where I would name it as btr instead.

Further more, definition of custom function void (*delay10ms_fn)() is wrong. Should be void (*delay10ms_fn)(void) instead, and even better, make a typedef instead. But do we really need 10ms delay every time?

[mvfpoa]

Function ow_ds2430_storage_write_scratchpad_raw returns owr_t type, and should use OW_ASSERT(), not OW_ASSERT0() as it is now. Same goes for function ow_ds2430_storage_write_scratchpad and some others. Didn't check all cases.

Now I understand. I haven't seen the content of ASSERT0.

You implementation if function ow_read_rom_raw could be written with less return statements. See how it is implemented for DS18X20 driver. Nested if statements.

I will have a look

Function ow_ds2430_storage_write_scratchpad has length parameter

I suspected you were talking about it. Changing this parameters name to btw is preferred.

Should be void (*delay10ms_fn)(void) instead, and even better, make a typedef instead. But do we really need 10ms delay every time?

I will typedef this. The reason for this forced 10 ms delay is datasheet indicates the data line must be help up for at least 10 ms which is the device programming time. If ow_ds2430_storage_copy_from_scratchpad function returns before time expiration, any attempt to issue a new command to this device or some other bus device may cause the background programming process to fail. Is this explanation reasonable? A negative point is this delay does not guarantee data line up. This command is performed only once per device, as it locks the eprom.

As soon as I review, I will send you a new message.

Best regards Márcio