sebi2k1 / node-can

NodeJS SocketCAN extension
215 stars 72 forks source link

removeListener? #91

Open fabdrol opened 3 years ago

fabdrol commented 3 years ago

AFAIK there's no way to call removeListener on a channel. This poses some questions:

sebi2k1 commented 3 years ago

The listener is associated to the channel objects itself regardless of its state.

Adding/removing listeners after starting it was not considered, let me check.

aufabdulnafea commented 9 months ago

Hello,

Thank you very much for the great library. However, I need to be able to remove the listener from a RawChannel, as I am implementing the UDS protocol. I want to add a listener for each UDS request and then remove it when the response is completed or if it's interrupted.

aufabdulnafea commented 9 months ago

this is the uds function if it helps.

uds(id: number, data: number[]): Promise<IUdsResult> {
    // this._validateData(data);

    data = data.concat(Array(8).fill(0x00)).slice(0, 8);
    const service = data[1];
    const expectedServiceId = 0x40 + service;

    if (data.length > 8) throw new Error("the data length should not exceed 8");

    // add padding (0x00) if the length of the data is less than 8

    return new Promise((resolve, reject) => {
      let responseLength = 0;
      let response: number[] = [];

      let nextSequentialNumber = 0;

      this._channel.addListener("onMessage", (message: any) => {
        if (message.id !== 0x7d1) return;
        const data = [...message.data];

        const frameType = (data[0] & 0xf0) >> 4;
        if (frameType > 2 || frameType < 0) return;

        // single frame (SF)
        if (frameType === 0) {
          if (data[1] !== expectedServiceId) return;
          responseLength = data[0] & 0xf;
          response = data.slice(1, Math.min(responseLength + 2, 8));
          return resolve({
            data: response,
            parsed: this._udsParser(response),
          });
        }

        // first frame (FF)
        else if (frameType === 1) {
          if (data[2] !== expectedServiceId) return;
          responseLength = ((data[0] & 0xf) << 8) | data[1];
          response = data.slice(2, 8);
          nextSequentialNumber = 1;
          this.send(id, [0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]);
        }

        // Consecutive Frame (CF)
        else if (frameType === 2) {
          const currentSequence = data[0] & 0xf;
          if (nextSequentialNumber !== currentSequence)
            return reject("the sequence of the request is wrong");

          nextSequentialNumber += 1;
          if (nextSequentialNumber > 0xf) nextSequentialNumber = 0;

          response = [
            ...response,
            ...data.slice(1, Math.min(responseLength - response.length + 1, 8)),
          ];

          if (responseLength - response.length === 0) {
            return resolve({
              data: response,
              parsed: this._udsParser(response),
            });
          }
        }
      });
      this.send(id, data);
    });
  }
juleq commented 9 months ago

I do not speak for the maintainer but it would be effort to add removal since it is in the native part of the code.

Could you elaborate on your use case? Why would retaining an active/inactive flag for the listener not do the trick? If you find it to be inactive, you just early return.

aufabdulnafea commented 9 months ago

Hi @juleq thanks for you response, you suggestion could do the trick, I still need to change the expected ID, but I thought it would be cleaner to remove the listener. The Signal according to the documentation has a removeListener method, but it is not clear to me if I can use the signals in my case

juleq commented 9 months ago

Since you do not use signal based communication but a higher level one, using a (fake) signal would be kind of hacky.

I would stick to the onMessage listener.

juleq commented 9 months ago

If, at some point, you want to venture into playing with the native code, I am sure a PR with a straightforward implementation of removeListener on the channel would be accepted.

Kheirlb commented 4 months ago

I think I am running into a possible similar issue. I would like to be able to dynamically switch between can0 and vcan0. I assume that is not an option?

export const getDatabaseService = (canInterface: string): DatabaseService => {
  const network = parseNetworkDescription("kr.kcd");
  const channel = createRawChannel(canInterface);
  channel.start();
  const kr_bus = new DatabaseService(channel, network.buses["kr"]);
  // setup listeners
  return kr_bus;
}

// initial setup on vcan0
let gKrBus = getDatabaseService("vcan0")
// later a switch to can0
gKrBus = getDatabaseService("ccan0")

Unfortunately, I was still seeing data from the callback functions setup during the vcan0 step. I can restructure to have some sort of flag as juleq suggested.

sebi2k1 commented 4 months ago

Mh, the way you built should actually work. What you can’t do, at least which interferes with these signal/database and CAN in general, is the send the same message from multiple nodes.

can you share more details what is happening?

As long as you have dedicated database per channel they work concurrently and depending on your network configuration (can0 and can1) are connected to the same bus you should be able to exchange signals between both CAN interface. But again, never send the same frames from different node. That usually not the way CAN and those node/bus concepts are used.