Mobsya / aseba-target-thymio2

Thymio 2 firmware
8 stars 8 forks source link

With wireless Thymios, illegal Aseba messages are generated for events of 40 values or longer #22

Closed stephanemagnenat closed 7 years ago

stephanemagnenat commented 7 years ago

The following program generates invalid Aseba messages on the desktop side, if N=40 or bigger. This leads Aseba Studio to exit following sanity check assertion in the message library. For N=5 the program works correctly.

# initialize map
var map[N]
var i = 0

while  i < N do
    map[i] = 1
    i = i + 1
end

# send map as an event
onevent button.left
emit sendMap map

I suspect that the RF serialization layer does not handle properly exceedingly large user messages.

This problem was initially reported in Aseba as aseba-community/aseba#619.

retornaz commented 7 years ago

wireless thymio is limited to 32 arguments.

stephanemagnenat commented 7 years ago

Yes, but it should not generate invalid Aseba messages if the user uses longer events, it should for instance cleanly truncate messages to 32 arguments.

Or it could generate an exception. This could be done by emitting a "node specific error" in the firmware of the Thymio if the user tries to send a message of more than 32 arguments. This might break some extreme programs, but it would avoid crashes and ensure consistency between wired and wireless Thymios.

mbonani commented 7 years ago

on USB the limit is 258. In the simulator it is not accessible due to the memory limitation. What is the rule about that?

mbonani commented 7 years ago

I was compile without ASEBA_ASSERT option. Should I compile with? I tried and didn't get message in studio using buffer over 258. Do I miss something?

stephanemagnenat commented 7 years ago

In the simulator it is not accessible due to the memory limitation.

What do you mean?

I was compile without ASEBA_ASSERT option. Should I compile with?

You mean the firmware?

I tried and didn't get message in studio using buffer over 258. Do I miss something?

You need to give more context.

mbonani commented 7 years ago

1) you cannot generate a wrong message with the simulator because it as the same memory size as Thymio.

2) yes, we are in a aseba-target-thymio2 issue

3) I try to compile with the Aseba_assert and it should send:

`#ifdef ASEBA_ASSERT if (length > ASEBA_MAX_EVENT_ARG_SIZE) AsebaAssert(vm, ASEBA_ASSERT_EMIT_BUFFER_TOO_LONG);

endif`

when I try to send buffer size over 258 (Thymio connect trough USB), I did see nothing in studio. Actually you can generate message with 258 arg, but not biggers. If we generate an exception for Wireless we should also make it for USB. Also why could not the aseba compiler warn about that...

mbonani commented 7 years ago

we cannot implement a "node specific error" as you suggest me this morning in "AsebaSendBuffer" because it is used for "user" and "debug" messages that can be 106 bytes long. The limitation of 32 argument is then only implemented in the Wireless for the "user" messages. Or I have to analyse the type of messages like in the Wireless module ...

davidjsherman commented 7 years ago

@retornaz do I understand that this is because, currently, an Aseba message must fit into a single unicast packet?

If the RF communication can use Contiki's reliable unicast (net/rime/runicast.h), then we can send large Aseba messages in more than one packet.

retornaz commented 7 years ago

@davidjsherman No, the message needs to fit in a single broadcast packet. Event are broadcasted and debug message are unicast and send only to the PC. Debug messages can be larger and are fragmented by the RF layer (IIRC, I did not looked at the code since 4 years)

stephanemagnenat commented 7 years ago

Or I have to analyse the type of messages like in the Wireless module ...

Yes, it is just checking the message type (uint16_t) being lower than 0x8000.

davidjsherman commented 7 years ago

@retornaz I was hoping that we could fragment longer messages into several runicasts, without retrofitting all of the dongles. (Rather than promoting the whole stack to uIP...).

Fragmentation of long messages could be handled in the Aseba layer if necessary but it seems contrary to the spirit.

mbonani commented 7 years ago

I just read back the code of the dongle and their is a return PACKET_NON missing:


  // This is an user packet ... check if the size is correct
  if(hdr[0] > 6 + MAX_USER_DATA_COUT*2) {
    putstring("Drop usr\n");
    fifo_drop(f,hdr[0]);
    leds_on(LEDS_RED);
  }
  return PACKET_USER;

but in the module it should work so I don't understand why this packets are passing

  // This is an user packet ... check if the size is correct
  if(hdr[0] > 6 + MAX_USER_DATA_COUT*2) {
    fifo_drop(f,hdr[0]);
    putstring("Dropping U\n");
    return PACKET_NONE;
    // FIXME Raise some flag ? 
  } 
  return PACKET_USER;
retornaz commented 7 years ago

@davidjsherman No because aseba event are not unitcast but multicast. Doing reliable multicast fragmentation is really really really hard.

mbonani commented 7 years ago

should be fix in 47b2f88135a230c9f4c40ace1b531654a89302e4

stephanemagnenat commented 7 years ago

So you decided to only generate the error when using RF? This is a problem because some code would work with USB and not with RF, while the change can occur live.

mbonani commented 7 years ago

For me it's clearly a Wireless Thymio limitation. Than I try both and it was not shocking me because you get the node error message. I will get some feedback around me. I understand also your point of view.

mbonani commented 7 years ago

Around me people are for coherence. I will change.

mbonani commented 7 years ago

fix in 0687bd2dfd991b2def29d056586c38d848451fa5

mbonani commented 7 years ago

finally fix in a926bccbba293940ab0a46cf09e42f2716669fd8