lexus2k/tinyproto

IFd class does not allow station address intiailization.

r2r0 opened this issue · 5 comments

r2r0 commented

We use IFd for both side of connection and IFd initializes station address with 0 (HDLC_E_BIT).
In the ABM mode with 2 stations, it works somehow - both stations have address 0 and they send frames to peer (also) with address 0.
But if we will try to add another station then we will need to assign addresses explicitly which is not possible with current IFd implementation.

Is the above reasoning valid?
If yes, ten would it be possible to add to IFd appropriate update for station address initialization?

Hi @r2r0

But if we will try to add another station then we will need to assign addresses explicitly which is not possible with current IFd implementation.

Yes, you're absolutely correct. IFd interface doesn't have support for NRM mode yet. The support of NRM mode is added only to C-style API for now.

If yes, ten would it be possible to add to IFd appropriate update for station address initialization?

Yes, it is possible. It will take some time.

r2r0 commented

Hi @lexus2k ,

Thanks for your answer.
After reading your explanation I realized that the origin of my problem may be not addressing, but mode initialization.
The init structure used in IFd constructor has default initialization for mode field and this causes mode to be initilized to 0 (TINY_FD_MODE_ABM).
Do you think that it was good approach to explicitly initialize mode field with TINY_FD_MODE_NRM?

FYI - the origin of the problem is that I use th newest tinyproto in the device and "old" version (from November) in the host and they are not able to communicate. After recompilation of host application communication was established but content of synchronization frames is different (addresses), so it worried me a little and I started poking around.

Hello, again

After reading your explanation I realized that the origin of my problem may be not addressing, but mode initialization.
The init structure used in IFd constructor has default initialization for mode field and this causes mode to be initilized to 0 (TINY_FD_MODE_ABM).

The previous implementation of tinyproto had been using 0xFF as address field for ABM mode. but according to official RFC documents, stations in ABM mode must use 0x01 or 0x03 as address field value (that corresponds to addres 0x00, because 2 lower bits have special meaning).
Of course, if you need I can make dedicated branch for you with old address field value 0xFF.

static uint8_t __address_field_to_peer(tiny_fd_handle_t handle, uint8_t address)
{
    // Always clear C/R bit (0x00000010) when comparing addresses
    address &= (~HDLC_CR_BIT);
    // Exit, if extension bit is not set.
    if ( !(address & HDLC_E_BIT) )
    {
        // We do not support this format for now.
        return 0xFF;
    }
    // If our station is SECONDARY station, then we must check that the frame is for us
    if ( __is_secondary_station( handle ) || handle->mode == TINY_FD_MODE_ABM )
    {
        // Check that the frame is for us
        return address == handle->addr ? 0 : 0xFF; // <<<<<<< This check may affect your case, just change to return 0;
    }
    // This code works only for primary station in NRM mode
    for ( uint8_t peer = 0; peer < handle->peers_count; peer++ )
    {
        if ( handle->peers[peer].addr == address )
        {
            return peer;
        }
    }
    // Return "NOT found peer"
    return 0xFF;
}

Do you think that it was good approach to explicitly initialize mode field with TINY_FD_MODE_NRM?

To advise you something useful, I need to have more details on what you're trying to implement. Originally tinyproto was intended only for peer to peer connections. One to many is the new feature, and it has its own limitations.

r2r0 commented

Ok, it is clear now, thank you very much.
As I wrote origin of this issue was incomaptibility of "old" and "new" implementations, but after cause is now well known it is no longer a big problem for me.
If needed I will manage my own branch with above code.

One to many future is for now only in my plans and depends also on hardware changes, so after I will have better picture of my need I will contact you.

BTW> If IFd is supposed to handle ABM only, then maybe it will be good idea to explicitely state that and add to IFd::begin:

init.mode = TINY_FD_MODE_ABM;

If IFd is supposed to handle ABM only, then maybe it will be good idea to explicitely state that and add to IFd::begin: init.mode = TINY_FD_MODE_ABM;

Good idea.