Comment by sebastien.lelong:
General Comment:
Hi Matt,
Some comments about your libs. Maybe there are duplicates of what you
lately mentioned in jallib thread.
Cheers,
Seb
Line-by-line comments:
File: /trunk/include/networking/ethernet_mac.jal (r2437)
===============================================================================
Line 26: procedure ethernet_header_create(byte in type, byte in mac_0, byte
in mac_1, byte in mac_2, byte in mac_3, byte in mac_4, byte in mac_5) is
-------------------------------------------------------------------------------
About "type" argument. I understand it's used to specify the last byte of
the two-byte ethertype field. First byte is hard-coded to 0x08. "type" can
be 0x00 so ethertype = 0x0800 for IPv4 frame (eg ICMP). It can be 0x06 to
match ARP protocol where ethertype is 0x0806.
Why didn't you allow "type" to be a word and let callee specify the full
ethertype value ? ethernet_header_create() is called with named
constant "ARP" within arp.jal lib, and with not-so-meaningful 0 value
within icmp.jal lib for instance. Does it make more sense to say
ethernet_header_create(IPv4,...) or ethernet_header_create(ARP,...) and
define const word IPv4 = 0x0800 and const word ARP = 0x0806. It think it
would be easier to follow/read the code, and would also allow other
ethertype to be implemented (who knows...)
Line 27: -- set source and destination MAC addresses
-------------------------------------------------------------------------------
Nitpicking... Follow ethernet frame fields order: first destination, then
source. this way reader can network_data[0], ...[1], ..., ...[5], and then
better understand why you have an offset of 6 to set source address.
As I said, nitpicking...
File: /trunk/include/networking/network_globals.jal (r2437)
===============================================================================
Line 49: if NETWORK_ENC28J60 == TRUE then
-------------------------------------------------------------------------------
Same remark as for NETWORK_LINK_LAYER, if you have multiple ethernet
modules, other than ENC28J60, True/False approach won't work anymore.
File: /trunk/include/networking/network_main.jal (r2437)
===============================================================================
Line 71: if NETWORK_LINK_LAYER_ETHERNET == FALSE then
-------------------------------------------------------------------------------
From what I understand, when NETWORK_LINK_LAYER_ETHERNET is False, this
means you're using SLIP. If another protocol is implements, this True/False
approach won't work anymore (or with a lot of "if").
What about checking NETWORK_LINK_LAYER = ETHERNET / SLIP, that is, checking
against some arbitrary constant value ?
For more information:
http://code.google.com/p/jallib/source/detail?r=2437
--
You received this message because you are subscribed to the Google Groups
"jallib" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/jallib?hl=en.
