search

lancaster-university / codal-microbit-v2

CODAL target for the micro:bit v2.x series of devices
MIT License
41 stars 50 forks source link

Set MICROBIT_RADIO_MAX_PACKET_SIZE as a configurable value. #387

Closed microbit-carlos closed 7 months ago

microbit-carlos commented 8 months ago

Set as a draft PR as I haven't tested this yet. I'll do that as part of some radio bridge work were this will be needed.

github-actions[bot] commented 8 months ago

Build diff

Base commit: 1370026f4bfbf96b204fa901f98d1f166f91b078 Action run: https://github.com/lancaster-university/codal-microbit-v2/actions/runs/6697395813

     VM SIZE    
 -------------- 
  [ = ]       0    TOTAL
github-actions[bot] commented 7 months ago

Build diff

Base commit: 1bb648e00da9204b6e707cc71248f74e2c0e3b95 Action run: https://github.com/lancaster-university/codal-microbit-v2/actions/runs/6797543953

     VM SIZE    
 -------------- 
  [ = ]       0    TOTAL
microbit-carlos commented 7 months ago

Tested and ready for review @JohnVidler

CI failures are due to:

JohnVidler commented 7 months ago

Just double checked the math for the radio and this looks fine, although perhaps some rationale on the 250B max is needed as a comment with the config var declaration?

It looks like send() checks if the total payload size would actually fit in a frame, so as long as we don't have over about 15B of additional header we should be ok, but this is all presuming that we always have a 256B absolute maximum radio packet size.

microbit-carlos commented 7 months ago

Thanks @JohnVidler! If you have a definitive answer as to how many "user bytes" we can fit in a radio datagram, we can update the comment pointing to https://github.com/lancaster-university/codal-microbit-v2/issues/383 and set the right value.

If not, it'd be great to get this merged first so that it can be present in the next release as I'm currently using it to the ml programme (building from a different branch) and will need it as well for the radio bridge which I'll be starting very soon.

JohnVidler commented 7 months ago

As this is needed right now lets merge this as it stands, then I’ll update the config variable comments later with a rationale for the 250B value afterwards, then you can continue on master rather than a branch.

microbit-carlos commented 7 months ago

Great, thanks @JohnVidler!

We need to update these comments and values as part of whatever ends up being the resolution of https://github.com/lancaster-university/codal-microbit-v2/issues/383 anyway, so I'd update the comments as part of that.