Davkim add el6001 #71
Conversation
There was a problem hiding this comment.
Thanks for the contribution David!
- Consider simplifying the Rx/TxPDOs to use a
uint8_t[22]for ease of use. - Consider modifying the write functions to accept an array up to 22 bytes rather than specifying a specific index in the array - this feels error prone and awkward to me.
- Multiple writes would append data up to the cap of 22 bytes
src/jsd_el6001_pub.h
Outdated
| int jsd_el6001_set_transmit_data_8bits(jsd_t* self, uint16_t slave_id, int byte, uint8_t value); | ||
|
|
||
| int jsd_el6001_request_transmit_data(jsd_t* self, uint16_t slave_id, int num_bytes_to_transmit); | ||
|
|
||
| int jsd_el6001_set_persistent_transmit_data(jsd_t* self, uint16_t slave_id, int num_bytes_to_transmit, bool state); |
There was a problem hiding this comment.
Please document these interfaces.
I'd expect you to have API write function of the form:
write(..., const char* data, size_t data_length)
// or possibly
write(..., const uint8_t* data, size_t data_length)
In the same flavor has memcpy, memset, or strncpy
There was a problem hiding this comment.
I left this functionality as fastcat level command, wonder that would be sufficient.
There was a problem hiding this comment.
So that function calls this function up to 22 times? Why?
|
@davidinkyu-kim What is the status of this driver - is it ready to do a final round of testing and merge or should we take it back to draft status? |
|
I think we are pending on loopback test: to test Tx/Rx latency, which would be not critical at the moment. |
|
Let's wait until the loopback test can be completed to merge, else it may get forgotten. Do you need help with it? If you can help me find a spare EL6001 module, I'll offer to test it. |
This reverts commit d4fff0b.
|
@alex-brinkman loopback test has been done. Confirmed that we can transmit/receive at one loop cycle persistently. |
| // Set for user to know number of bytes received | ||
| state->num_bytes_received = num_bytes_received; | ||
|
|
||
| return ok ? 0 : -1; |
There was a problem hiding this comment.
This variable, "ok," is never false. Is that okay?
src/jsd_el6001_types.h
Outdated
| JSD_EL6001_DATA_OUT_BYTE_21, | ||
| JSD_EL6001_NUM_PDO_ENTRIES_OUTPUT, | ||
|
|
||
| } jsd_el6001_rxpdo_entries_t; |
There was a problem hiding this comment.
I don't believe this is used anywhere in the final version, omit
src/jsd_el6001_pub.h
Outdated
| int jsd_el6001_set_transmit_data_8bits(jsd_t* self, uint16_t slave_id, int byte, uint8_t value); | ||
|
|
||
| int jsd_el6001_request_transmit_data(jsd_t* self, uint16_t slave_id, int num_bytes_to_transmit); | ||
|
|
||
| int jsd_el6001_set_persistent_transmit_data(jsd_t* self, uint16_t slave_id, bool is_persistent); |
There was a problem hiding this comment.
Document these.
Why is there no function akin to memcpy with an array of uint8_t and n_bytes? for example,
bool jsd_el6001_write(jsd_t* self, uint8_t* data_in, size_t data_len);
src/jsd_el6001_types.h
Outdated
| jsd_el6001_sms_t sms; | ||
| jsd_el6001_transmit_sms_t transmit_state; | ||
|
|
||
| } jsd_el6001_state_t; |
There was a problem hiding this comment.
Since this is in the public API, it needs to be explained better. As a causal user, I don't know how to read this and get my data out of the returned state struct without digging into the implementation.
in the EGD class, I introduced a notion of public and private state (to help shield the public interface from unnecessary cruft) Could that pattern be employed here perhaps?
consider adding an fread-like function to return validated user data.
src/jsd_el6001_pub.h
Outdated
| * @param slave_id Slave ID of EL6001 device | ||
| * @return Pointer to EL6001 device state | ||
| */ | ||
| const jsd_el6001_state_t* jsd_el6001_get_state(jsd_t* self, uint16_t slave_id); |
There was a problem hiding this comment.
Consider offering an fread like interface where the user passes a buffer to copy the recv data into.
|
I don't understand what is meant by persistence in the transmit cycle, can you explain that? This is probably related to the |
|
Persistent transmit can be set on public function call, making terminal to persistently send out current outgoing buffer every cycle. |
|
@alex-brinkman Thank you for the feedback. I've pushed few commits, would appreciate if you can review them. |
Added jsd_el6001 driver