slave.cpp ``setPacketBuffer`` typo?

[sonelu]

In slave.cpp I think there is typo in the setPacketBuffer:

bool 
Slave::setPacketBuffer(uint8_t* p_buf, uint16_t buf_capacity)
{
  if(p_packet_buf_ == nullptr){
    last_lib_err_ = DXL_LIB_ERROR_NULLPTR;
    return false;

...

shouldn't this be instead:

bool 
Slave::setPacketBuffer(uint8_t* p_buf, uint16_t buf_capacity)
{
  if(p_buf == nullptr){
    last_lib_err_ = DXL_LIB_ERROR_NULLPTR;
    return false;

...

The check should be against p_buf argument and not against p_packet_buf_ attribute?

[ROBOTIS-Will]

Hi @sonelu The conditional statement you mentioned is checking whether the packet buffer pointer is created successfully or not so that p_buf can be used for p_packetbuf

[sonelu]

Hi @ROBOTIS-Will,

Sorry, but I really don't see the point of that.

The following code will work fine and would practically brick the Slave instance:

slave.setPacketBuffer(nullptr, 10);

Any subsequent calls to fix the buffer after that will fail because of that check:

buff_new = new unit8_t[20];
slave.setPacketBuffer(buff_new, 20);

Am I missing something?

[ROBOTIS-Will]

Hi @sonelu , I don't think anyone would intentionally pass the null pointer to the setPacketBuffer function, but in order to achieve safer code, you may append validation process for the parameters. @OpusK Any thoughts on this issue?

[OpusK]

@sonelu , @ROBOTIS-Will

Oh, sorry guys. It is just a typo... p_buf is correct.

[ROBOTIS-Will]

@OpusK Thank you for confirming this issue.

[ROBOTIS-Will]

This issue will be closed with the fix #51 . Please feel free to reopen when necessary.