[RFC] Add PF_CAN protocol for socket CAN #11454

vipinana · 2018-11-17T10:46:44Z

This patch adds support for PF_CAN protocol and CAN_RAW type sockets, also it adds common APIs for socket can driver and header file.

pfalcon

Thanks for the patch. But even quick look shows that it would need to be reworked considerably before it may become suitable for the mainline.

Actually, the problem is with the mainline itself - it's not extensible, so it's not possible to add non-standard POSIX types with violating/compromising very shaky layering we already have. So, I encourage you to start with step by step refactoring Zephyr networking stack to be able to accommodate extensions like that. (That's not easy a task.)

See also #11229

pfalcon · 2018-11-17T10:58:51Z

subsys/net/ip/net_context.c

@@ -129,6 +129,13 @@ int net_context_get(sa_family_t family,
 		return -EPFNOSUPPORT;
 	}
 #endif
+#if !defined(CONFIG_SOCKET_CAN)


Please see the name of the file which is patched here - subsys/net/ip/net_context.c . Is CAN an IP protocol? If not, then how it ends up in net/ip/ ?

Hi Paul,
Thanks for the comments.
To add PF_CAN protocol, it needs to re-use existing network layer of Zephyr. According to CAN CIA ORG it is mentioned that "CAN network device drivers
implement the same standardized networking driver model as Ethernet drivers".
Reference: https://www.can-cia.org/fileadmin/resources/documents/proceedings/2012_kleine-budde.pdf,

I don't think a document for Linux applies for Zephyr.

@pfalcon net/ip/ directory is actually wrongly named, I'd call it net/core/ instead.

@tbursztyka

@pfalcon net/ip/ directory is actually wrongly named, I'd call it net/core/ instead.

I don't agree, majority of content in net/ip/ is TCP/IP networking related. Perhaps, if we find that some non-IP networking shares some code with the IP one, we can split it to net/core/ . I don't think there would be a lot of such code though. (Partly because a lot of code in net/ip/ is subpar in needs to be rewritten, not shared.)

But that's not even the first question regarding this PR. The first question is whether CAN has anything to do with subsys/net at all. I'd say, CAN has much more in common with e.g. I2C than with e.g. Ethernet. We don't have I2C in subsys/net/, so why CAN goes there? Then if it does, I'm absolutely firm that it should not touch anything in net/ip/, and should be localized to net/l2/can/ and net/lib/sockets/. I love to be proven wrong of course ;-).

@pfalcon In order to work with socket, CAN needs to integrate with net_if, net_context, having an l2 etc... thus why it has stuff in net. Also, in a "short future", socket won't be a library anymore but part of the net core (thus why at some point, net/ip is going to be more than ip stuff only).

In order to work with socket, CAN needs to integrate with net_if, net_context, having an l2 etc...

Not really - see e.g. how TLS sockets are implemented, they didn't patch anything in net/ip/. And @rlubos now does great work of refactoring socket dispatching to allow even easier integration of different socket types (as part of #11438).

Also, in a "short future", socket won't be a library anymore but part of the net core (thus why at some point, net/ip is going to be more than ip stuff only).

Not the way I see it. As I see it, net_context is already a legacy implementation detail of TCP/UDP networking. In some (not so near) future, when net_context is deprecated from being a public interface, it can change drastically, to support more efficient TCP/UDP networking for sockets. But sockets will remain a separate multiplexing layer on top of it, exactly because of the need to support disparate protocol implementations thru them. These protocol implementations should remain separate however, and not lumped one into another.

pfalcon · 2018-11-17T11:00:38Z

include/net/socket_can.h

+	struct device *can_dev;
+};
+
+__syscall int socket_can_ioctl(struct device *dev, u32_t cmd, void *data);


Sockets is purely a POSIX construct. For as long as POSIX doesn't have function socket_can_ioctl(), it doesn't make sense to have it here too.

These calls will be replaced by setSocketOpt and getSocketOpt call, currently these calls are not implemented in Zephyr hence need to use customized ioctls calls.

getsockopt and setsockopt are there already, afaik.

currently these calls returning -1 and have no logic implemented.

zephyr/subsys/net/lib/sockets/sockets.c : 841-- int zsock_getsockopt(int sock, int level, int optname, void *optval, socklen_t *optlen) { errno = ENOPROTOOPT; return -1; } int zsock_setsockopt(int sock, int level, int optname, const void *optval, socklen_t optlen) { errno = ENOPROTOOPT; return -1; }

It's ok: implement what you need in these functions to make it work only for your type of socket protocol. (and rest should still set the errno and return -1 as unsupported)

pfalcon · 2018-11-17T11:01:12Z

include/net/socket_can.h

+
+	return api->ioctl(dev, cmd, data);
+}
+__syscall int socket_can_check_filter_matched(struct device *dev,


Similar comment to the above - no socket_can_check_filter_matched() function in POSIX.

@pfalcon: What if we get this patch in as-is and update it as soon as you'll be ready with setsockopt code?

@laperie that's not a good idea actually

Why do you actually need a function like socket_can_check_filter_matched?
If you don't set a filter that matches, the CAN driver will never receive a message and therefore if you get a message from the can API it passed the filter already.

@alexanderwachter This function implements local loopback functionality mentioned in section 3.2 of https://www.kernel.org/doc/Documentation/networking/can.txt. And in https://www.can-cia.org/fileadmin/resources/documents/proceedings/2012_kleine-budde.pdf document it is mentioned to implement it via software filtering. So this function checks if sent CAN packet Id is matched with any of configured filter id on sender node then socket application will receive this locally loopbacked packet.

Be aware that the loopback mode bit does not only check the filter and send the message back if it matches. It also set the ACK bit while sending. Basically it acknowledges its own package.
This makes a difference when your device is alone on the bus. If you do not set the loopback mode here, the controller will try to send the message forever.

This loopback is not via can controller. It will check the matched filter in SW layer only and if matched then connection callback will be trigger with this pkt and socket recv call will get the packet. So for every sent packet this check will happen and if current mode of CAN is loopback mode then same mode will receive two packets. One from CAN controller and another from sw filtering.

Removed ioctl calls and implemented getsockopt and setsockopt APIs for configuring can mode, bit rate and filters.

pfalcon · 2018-11-17T11:22:34Z

So, I encourage you to start with step by step refactoring Zephyr networking stack to be able to accommodate extensions like that. (That's not easy a task.)

Actually, that's exaggeration. We do have framework how to add new socket types - please see for example TLS sockets or socket offload. With basic infrastructure like that, adding new socket types does not require patching unrelated files like net/ip/ . That infrastructure is indeed basic though, so someone adding a new socket type should be prepared to do some refactoring of that infrastructure, in cooperation with the other socket developers.

codecov-io · 2018-11-17T11:26:10Z

Codecov Report

Merging #11454 into master will increase coverage by 1.69%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11454      +/-   ##
==========================================
+ Coverage   48.21%   49.91%   +1.69%     
==========================================
  Files         279      194      -85     
  Lines       43303    24830   -18473     
  Branches    10370     5163    -5207     
==========================================
- Hits        20878    12393    -8485     
+ Misses      18279    10509    -7770     
+ Partials     4146     1928    -2218

Impacted Files	Coverage Δ
lib/fdtable.c	`0% <0%> (-30.44%)`	⬇️
include/net/buf.h	`76.92% <0%> (-23.08%)`	⬇️
include/misc/slist.h	`86.66% <0%> (-6.67%)`	⬇️
subsys/net/buf.c	`56.91% <0%> (-4.25%)`	⬇️
kernel/sched.c	`91.75% <0%> (-1.04%)`	⬇️
include/misc/byteorder.h	`96.87% <0%> (-0.86%)`	⬇️
subsys/bluetooth/host/conn.c	`32.38% <0%> (-0.1%)`	⬇️
include/kernel.h	`98.43% <0%> (-0.05%)`	⬇️
arch/posix/include/kernel_arch_func.h	`100% <0%> (ø)`	⬆️
soc/posix/inf_clock/soc.c	`100% <0%> (ø)`	⬆️
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0abd922...93b7a95. Read the comment docs.

pfalcon · 2018-11-21T08:04:47Z

Sorry, swamped with other things to provide detailed replies so far. Let me just ping @rlubos and @GAnthony who work on alternative socket implementations, so they both were in loop that we potentially have another socket family coming, and perhaps provided suggestions too.

@vipinanand, you can also in your turn review @rlubos' #11438 . That's supposed to be a good step toward "doing it right" re: alternative socket implementations.

pfalcon · 2018-11-21T08:25:03Z

@laperie: Let me take this from #11454 (comment) and answer here, as it concerns general process approach:

@pfalcon: What if we get this patch in as-is and update it as soon as you'll be ready with setsockopt code?

Well, it's not exactly about "me". As an original author and maintainer of socket subsys, I can oversee and provide guidelines on ways to achieve things, but can't be a bottleneck on implementing them, i.e. other developers should be ready to step in and work on some parts of it. That works pretty well with @rlubos and @GAnthony, which provide - and elaborate - extension points and overall structure of socket subsys to cover wider usecases than the original implementation. We need more developers to join that cooperation.

Otherwise, we can't on one hand call to clean up the networking stack, and on the other, ask to merge not properly layered patches now and hope they will be fixed later - that doesn't make sense.

@laperie, thanks for stepping in otherwise, now it's clear that this comes from work overseen by you. But let me share my concern that we have deterioration in (publicly visible) project planning re: networking. At Jira times, we had detailed descriptions of features/requirements. After migrating to Github, the descriptions became progressively more bare. And the latest trending is that there're no task/requirements ticketing at all, step 1 is a code drop.

Sorry, but it's impossible to tell if a code does something right or wrong without knowing what it should do in the first place. Requirements come first, then code. That should be well know to any project manager, my point is that it's known even to developers, based on (painful) experience. I also assume that you're overbusy as everyone, so I post this note in case it helps you to assess if it's worth doing something, or nobody cares, and adjust the priority: tasks/tickets with (detailed) specs/requirements are absolutely needed, moreso as Zephyr continues to grow extensively and the front of works only increases. Thanks!

pfalcon · 2018-11-21T08:31:02Z

ready with setsockopt code?

Or another example going in parallel: #11522 . It may be not totally perfectalized yet, and we may need to refactor it further later, but people find a way to plug their setsockopt needs into existing API, instead of introducing other adhoc APIs (which will be a pain to deprecate later, and deprecation is the only road forward for such APIs).

tbursztyka

I would separate the addition of SOCK_RAW type and PF_CAN. Moreover knowing that this PR #11229 is attempting to add SOCK_RAW (not ready at all though).

Also, what is Signed-off-by: Zephyrus Zephyr zephyrus@zephyrproject.org in your commit message?
Seems like you have to remove this.

tbursztyka · 2018-11-21T08:44:27Z

include/net/net_context.h

@@ -27,6 +27,9 @@
 #include <net/net_if.h>
 #include <net/net_stats.h>

+#include <can.h>


you don't need to include that header

tbursztyka · 2018-11-21T08:47:52Z

include/net/net_context.h

@@ -353,7 +356,10 @@ static inline sa_family_t net_context_get_family(struct net_context *context)
 	if (context->flags & NET_CONTEXT_FAMILY) {
 		return AF_INET6;
 	}
-
+	if (IS_ENABLED(CONFIG_SOCKET_CAN)


There are 2 issues here:

indentation is a bit off: always indent below last unclosed '(' + 1 space, so here you would align the '&& (...' below if ( + 1 space. Your editor should be able to handle this (if it can apply linux code style) automatically, so you wouldn't have to do it by hand. (Note that you did it properly in connection.c for instance)

you don't need to pack lines together, make the code breath a bit: empty line bore an if, empty line after an '}' (unless it's another '}' or a else etc..). Note however: if an 'if' is testing the result of an assignment done right before, no need of an empty line then. Like:
foo = stuff();
if (foo) {
...

Apply these 2 rules everywhere where relevant in your patch

tbursztyka · 2018-11-21T08:51:07Z

include/net/net_ip.h


 /** Protocol numbers from IANA */
 enum net_ip_protocol {
 	IPPROTO_ICMP = 1,
 	IPPROTO_TCP = 6,
 	IPPROTO_UDP = 17,
 	IPPROTO_ICMPV6 = 58,
+	CAN_RAW = 252,


How CAN (and raw even), can be part of IP protocol? Is it transported through IP? Just asking, I don't know CAN.

"The SocketCAN concept extends the Berkeley sockets API in Linux by introducing a new protocol family, PF_CAN, that coexists with other protocol families like PF_INET for the Internet Protocol. The communication with the CAN bus is therefore done analogously to the use of the Internet Protocol via sockets."
Source: https://en.wikipedia.org/wiki/SocketCAN

Few comments here:

I'm relatively fine with this particular change, the problem can be seen as in enum naming.

BUT, linux has it as a separate define in can.h, and I would like to extend my suggestion is not to invent our own special ways and follow well-known/established API examples (and belonging of symbols to headers is part of API in C).

"The communication with the CAN bus is therefore done analogously to the use of the Internet Protocol via sockets." - "Analogously" does not mean "the same". Because otherwise, absolutely all protocols work analogously - shoving bits back and forth. But somehow, this patch patches the IP protocol files and not I2C or MQTT. Looking a bit down the same Wikipedia article, example shows that what is sent over CAN sockets is struct can_frame. This is very, very different from normal TCP or UDP sockets.

tbursztyka · 2018-11-21T08:52:51Z

include/net/socket_can.h

+	struct device *can_dev;
+};
+
+__syscall int socket_can_ioctl(struct device *dev, u32_t cmd, void *data);


It's ok: implement what you need in these functions to make it work only for your type of socket protocol. (and rest should still set the errno and return -1 as unsupported)

tbursztyka · 2018-11-21T08:56:09Z

subsys/net/ip/net_context.c

@@ -129,6 +129,13 @@ int net_context_get(sa_family_t family,
 		return -EPFNOSUPPORT;
 	}
 #endif
+#if !defined(CONFIG_SOCKET_CAN)


@pfalcon net/ip/ directory is actually wrongly named, I'd call it net/core/ instead.

tbursztyka · 2018-11-21T08:57:05Z

subsys/net/ip/net_context.c

@@ -155,31 +162,52 @@ int net_context_get(sa_family_t family,
 		return -EPROTONOSUPPORT;
 	}
 #endif
+#if !defined(CONFIG_SOCKET_CAN)
+	if (type == SOCK_RAW) {
+		NET_ASSERT_INFO((type != SOCK_RAW), "SCOKET RAW disabled");


typo in the string

tbursztyka · 2018-11-21T08:58:55Z

subsys/net/ip/net_context.c

@@ -155,31 +162,52 @@ int net_context_get(sa_family_t family,
 		return -EPROTONOSUPPORT;
 	}
 #endif
+#if !defined(CONFIG_SOCKET_CAN)


Use if (!IS_ENABLED(...))

tbursztyka · 2018-11-21T09:01:02Z

subsys/net/ip/net_context.c

+			return 0;
+		}
+		context->recv_cb = cb;
+		struct sockaddr local_addr = {


you cannot declare a variable in the middle of a code block, always at the beginning of it. So have a new code block opened in your CAN_RAW.

tbursztyka · 2018-11-21T09:03:40Z

subsys/net/lib/sockets/socket_can.c

+
+	if (pkt != NULL) {
+		ctx = net_pkt_context(pkt);
+		if (ctx != NULL) {


It's a bit convoluted: only test for error, not succes.
So it would be:

ctx = net_pkt_context(pkt);
if (ctx == NULL) {
goto err;
}

iface = net_context_get_iface(ctx);
if (iface == NULL) {
goto err;
}

etc...

tbursztyka · 2018-11-21T09:04:48Z

subsys/net/lib/sockets/sockets.c

@@ -680,7 +690,7 @@ int _impl_zsock_fcntl(int sock, int cmd, int flags)
 	switch (cmd) {
 	case F_GETFL:
 		if (sock_is_nonblock(ctx)) {
-		    return O_NONBLOCK;


don't fix that in this patch, it's not related at all. (you are not supposed to fix existing indentation issues). Feel free to send one separate if you want though.

tbursztyka · 2018-11-22T10:12:53Z

include/net/net_context.h

@@ -27,7 +27,6 @@
 #include <net/net_if.h>
 #include <net/net_stats.h>

-#include <can.h>


You should not add commit that fixes your previous commit.

just fixup the original commit.
git rebase -i HEAD~2

there you'll see your 2 commits, in front of that one "net: corrected indentation" replace "pick" by "f", save/exit and that should be done.

Then git -f push (-f for forcing the update)

And that's it.

tbursztyka · 2018-11-26T12:23:49Z

include/net/net_context.h

@@ -354,6 +356,11 @@ static inline sa_family_t net_context_get_family(struct net_context *context)
 		return AF_INET6;
 	}

+	if (IS_ENABLED(CONFIG_SOCKET_CAN) &&
+		(context->flags & CAN_CONTEXT_FAMILY)) {


still issue with indentation: it should be below last opened '(' + 1 space. So here, right below IS_...

zephyrbot · 2018-11-26T15:07:29Z

Found the following issues, please fix and resubmit:

Checkpatch issues

-:300: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (24, 24)
#300: FILE: subsys/net/ip/connection.c:604:
 			} else
[...]
+			if (IS_ENABLED(CONFIG_SOCKET_CAN) &&

-:312: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (24, 24)
#312: FILE: subsys/net/ip/connection.c:643:
 			} else
[...]
+			if (IS_ENABLED(CONFIG_SOCKET_CAN) &&

-:398: ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
#398: FILE: subsys/net/ip/net_context.c

-:511: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 8)
#511: FILE: subsys/net/ip/net_context.c:1036:
 	} else
[...]
+	if (IS_ENABLED(CONFIG_SOCKET_CAN) &&

-:629: WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 8)
#629: FILE: subsys/net/ip/net_core.c:254:
 	} else
[...]
+	if (IS_ENABLED(CONFIG_SOCKET_CAN) &&

-:636: WARNING:UNNECESSARY_ELSE: else is not generally useful after a break or return
#636: FILE: subsys/net/ip/net_core.c:259:
+		return 1;
+	} else {

- total: 1 errors, 5 warnings, 870 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

alexanderwachter · 2018-11-26T16:43:09Z

Thanks for extending the CAN API to BSD sockets!
Did you already thought about receiving messages? Actually the CAN can_attach_isr function should be sufficient to set the filter. I think the socket API for that should look something like this:
setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, &rfilter, sizeof(rfilter)) [1]?

[1] https://www.kernel.org/doc/Documentation/networking/can.txt

vipinana · 2018-11-27T04:23:08Z

Thanks for extending the CAN API to BSD sockets!
Did you already thought about receiving messages? Actually the CAN can_attach_isr function should be sufficient to set the filter. I think the socket API for that should look something like this:
setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, &rfilter, sizeof(rfilter)) [1]?

[1] https://www.kernel.org/doc/Documentation/networking/can.txt

@alexanderwachter The configuration of filter and receive path is implemented as part of vendor specific socket can driver. And currently filter configuration are done via customized ioctl call (will be replaced with setSocketOpt call) Currently that is not part of this this patch but below is the snapshot of filter callback-
void socket_can_<soc_name>_rx_callback(struct can_msg *msg)
{
struct net_pkt *pkt = NULL;
struct net_buf *buf = NULL;

pkt = net_pkt_get_reserve(&sock_pkts_slab, 0, K_NO_WAIT);
if (pkt == NULL) {
	return;
}
net_pkt_append(pkt, sizeof(struct can_msg), (void *)msg, K_NO_WAIT);
net_pkt_set_iface(pkt, socket_context[msg->can_id].iface);
can_conn_input(pkt, false);

}

static int socket_can_<soc_name>_ioctl(struct device *dev, u32_t cmd, void *data)
{
int ret = 0;
struct socket_can_context *context = dev->driver_data;
struct device *<soc_name>can_dev = context->can_dev;
struct can_filter *filter = NULL;
struct socket_can<soc_name>_mode *mode = NULL;

switch (cmd) {
case SOCKET_CAN_IOCTL_CONFIG_FILTER:
	ret = CAN_NO_FREE_FILTER;
	filter = (struct can_filter *)data;
	if (filter == NULL) {
		goto err;
	}
	ret = can_attach_isr(isesi_can_dev,
			     &socket_can_<soc_name>_rx_callback,
			     filter);
	break;
case SOCKET_CAN_IOCTL_CONFIG_MODE:
	mode = (struct socket_can_<soc_name>_mode *)data;
	if (mode == NULL) {
		goto err;
	}
	ret = can_configure(isesi_can_dev,
			    mode->op_mode, mode->bit_rate);
	break;
case SOCKET_CAN_IOCTL_GET_IF_INDEX:
	*((u8_t *)data) = net_if_get_by_iface(context->iface);
	break;
default:
	ret = can_ioctl(isesi_can_dev, cmd, data);
}

err:
return ret;
}

alexanderwachter · 2018-11-27T08:15:49Z

A comment regarding the name space can_l2:
Isn't it better to name it can_sock or can_raw?
For me can_l2 denotes that it is related to ISO/OSI layer 2 which it isn't.

pfalcon · 2018-11-27T09:18:26Z

A comment regarding the name space can_l2: Isn't it better to name it can_sock or can_raw? For me can_l2 denotes that it is related to ISO/OSI layer 2 which it isn't.

Which layer is it then? Note that in Zephyr, "l2" means not just "strictly corresponds to OSI layer 2", but rather "similar to other things already in Zephyr's l2". subsys/net/l2 doesn't have anything "IP" in it, so rules for adding to it is relaxed. (Whereas otherwise it's major concern with this PR as it is that it patches in unrelated protocol into subsys/net/ip).

This is not to claim that anything should go into subsys/net/l2, just that I personally have less concerns about it than about other things here.

But note that names like l2/can/can_l2.c look weird. Why "l2" is twice there?

tbursztyka · 2018-11-27T09:58:18Z

subsys/net/l2/can/can_l2.c

@@ -0,0 +1,40 @@
+/*


Actually, I don't see the need for a CAN L2. Current can device could just use the dummy l2.

Actually, I don't see the need for a CAN L2.

And how I see it, the whole of CAN is L2 (in Zephyr terms). Beyond that, just add multiplexing support to its (CAN's) routines on the level of sockets, voila. Summing up choices:

Make a separate subsys for CAN, subsys/can/. If done like that, can have an adhoc API in addition to Linux-like standard socket API.

Make it net/l2/can/.

this patch add support for PF_CAN protocol and CAN_RAW type sockets, also it adds common APIs for socket can driver and header file. Signed-off-by: Vipin Anand <vipin.anand@intel.com>

pfl · 2018-12-12T19:29:47Z

subsys/net/lib/sockets/socket_can.c

+	if ((buf == NULL) || (buf->data == NULL)) {
+		goto err;
+	}
+	msg = (struct can_msg *)(buf->data);


I'm not able to find the definition of struct can_msg. Is it part of this patch?

pfl · 2018-12-12T19:52:12Z

Is the intention to follow Linux SocketCAN, https://en.wikipedia.org/wiki/SocketCAN including the exact same protocol and address family numbers? If yes, then it makes sense to create a separate commit for this and only this issue.

If SocketCAN is followed, does it make sense to write only the payload bytes to the socket and having the socket abstraction to write the CAN address/identifier in front? I could not find this readily in the patch. If instead the whole CAN message with identifier and payload is written through the socket, making any kind of address/identifier in the CAN context unnecessary, as nothing will use it. This again makes CAN equal to raw sockets in #11229, and no further socket mangling is needed. If again SocketCAN is followed, then something useful needs to be done with the socket addresses, i.e. the CAN socket layer needs to be the one writing address/identifier, payload and CRC on behalf of the application. I don't see that happening in the patch right now.

How are the devices initialized, does anything special need to happen in the driver? How are the CAN drivers presented to Zephyr, I could not see any L2 interaction going on in the patch. Reading and writing might be as easy as using the dummy L2 as @tbursztyka pointed out, but the issue still remains how these L2 drivers are initialized and matched up with interfaces in order to be used by the socket implementation.

I'd like to see clarification on the above architectural issues before or while proceeding with this, perhaps there should be a architecture/design meeting about this; it might be quite difficult to initiate over email.

jukkar · 2019-01-29T07:25:55Z

Replaced by #12779 so closing this.

vipinana requested review from jukkar, pfalcon and tbursztyka as code owners November 17, 2018 10:46

pfalcon requested changes Nov 17, 2018

View reviewed changes

rveerama1 changed the title ~~net: socket: add PF_CAN protocol for socket can~~ [RFC] Add PF_CAN protocol for socket CAN Nov 19, 2018

rveerama1 self-requested a review November 19, 2018 07:27

rveerama1 added area: Networking RFC Request For Comments: want input from the community labels Nov 19, 2018

pfalcon added the area: Sockets Networking sockets label Nov 21, 2018

tbursztyka requested changes Nov 21, 2018

View reviewed changes

pfalcon mentioned this pull request Nov 21, 2018

sockets: tls: Make secure sockets support posix APIs #11438

Merged

tbursztyka reviewed Nov 22, 2018

View reviewed changes

vipinana force-pushed the add_socket_can_protocol_PF_CAN_support branch from 4c558ae to 8113149 Compare November 22, 2018 10:15

tbursztyka reviewed Nov 26, 2018

View reviewed changes

vipinana force-pushed the add_socket_can_protocol_PF_CAN_support branch from 8113149 to 1009074 Compare November 26, 2018 14:56

vipinana force-pushed the add_socket_can_protocol_PF_CAN_support branch from 1009074 to 712a401 Compare November 26, 2018 15:25

alexanderwachter added the area: CAN label Nov 26, 2018

vipinana force-pushed the add_socket_can_protocol_PF_CAN_support branch from 712a401 to 32bbae9 Compare November 27, 2018 04:33

tbursztyka reviewed Nov 27, 2018

View reviewed changes

vipinana force-pushed the add_socket_can_protocol_PF_CAN_support branch from 32bbae9 to 0f38af5 Compare November 28, 2018 10:50

net: socket: add PF_CAN protocol for socket can

93b7a95

this patch add support for PF_CAN protocol and CAN_RAW type sockets, also it adds common APIs for socket can driver and header file. Signed-off-by: Vipin Anand <vipin.anand@intel.com>

vipinana force-pushed the add_socket_can_protocol_PF_CAN_support branch 2 times, most recently from 79f6e42 to 93b7a95 Compare November 29, 2018 06:31

pfl suggested changes Dec 12, 2018

View reviewed changes

jukkar mentioned this pull request Jan 28, 2019

Generic socket CAN support #12779

Merged

jukkar closed this Jan 29, 2019

[RFC] Add PF_CAN protocol for socket CAN #11454

[RFC] Add PF_CAN protocol for socket CAN #11454

Conversation

vipinana commented Nov 17, 2018 • edited by rveerama1 Loading

pfalcon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vipinana Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfalcon Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vipinana Nov 20, 2018 • edited by nashif Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vipinana Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfalcon commented Nov 17, 2018

codecov-io commented Nov 17, 2018 • edited Loading

Codecov Report

pfalcon commented Nov 21, 2018

pfalcon commented Nov 21, 2018

pfalcon commented Nov 21, 2018

tbursztyka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zephyrbot commented Nov 26, 2018 • edited Loading

Checkpatch issues

alexanderwachter commented Nov 26, 2018

vipinana commented Nov 27, 2018

alexanderwachter commented Nov 27, 2018

pfalcon commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfl commented Dec 12, 2018 • edited Loading

jukkar commented Jan 29, 2019

vipinana commented Nov 17, 2018 •

edited by rveerama1

Loading

pfalcon left a comment •

edited

Loading

vipinana Nov 20, 2018 •

edited

Loading

pfalcon Nov 27, 2018 •

edited

Loading

vipinana Nov 20, 2018 •

edited by nashif

Loading

vipinana Nov 27, 2018 •

edited

Loading

codecov-io commented Nov 17, 2018 •

edited

Loading

zephyrbot commented Nov 26, 2018 •

edited

Loading

pfl commented Dec 12, 2018 •

edited

Loading