Skip to content

Conversation

@hf81-ble
Copy link

@hf81-ble hf81-ble commented Nov 5, 2025

  • added the Atronix wallbox on basis of the ocpp 1.6j implementation.
  • added Workarounds to handle with setting the output power
  • workaround to chargecurrent 0 => use remote stop
  • ocpp request are answered after getting BootNotification

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `charger/atronix.go:130` </location>
<code_context>
+	return c.OCPP.Enable(false)
+}
+
+func (c *Atronix) MaxCurrentMillis(current float64) error {
+	vendor, model := c.vendorModel()
+
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring MaxCurrentMillis by extracting logical blocks into helper functions to flatten control flow and reduce repetition.

Here’s a shorter, flatter version of MaxCurrentMillis that pulls each logical block into a helper. This preserves all behavior but removes deep nesting and repeated lookups:

```go
func (c *Atronix) MaxCurrentMillis(current float64) error {
    vendor, model := c.vendorModel()

    if !c.isAtronix(vendor, model) {
        return c.OCPP.MaxCurrentMillis(current)
    }

    if current <= 0 {
        return c.stopTransactionIfAny()
    }

    if err := c.ensureTransactionStarted(); err != nil {
        return err
    }

    if err := c.setChargeRate(current); err != nil {
        return err
    }

    return nil
}
```

And the new helpers:

```go
func (c *Atronix) isAtronix(vendor, model string) bool {
    return strings.Contains(vendor, "weeyu") || strings.Contains(model, "atronix")
}

func (c *Atronix) stopTransactionIfAny() error {
    txn, err := c.conn.TransactionID()
    if err != nil {
        return fmt.Errorf("ocpp/atronix: transaction lookup failed: %w", err)
    }
    if txn > 0 {
        if err := c.conn.RemoteStopTransactionRequest(txn); err != nil {
            return fmt.Errorf("ocpp/atronix: remote stop failed: %w", err)
        }
    }
    c.pendingStart, c.current, c.enabled = false, 0, false
    return nil
}

func (c *Atronix) ensureTransactionStarted() error {
    txn, err := c.conn.TransactionID()
    if err != nil {
        return fmt.Errorf("ocpp/atronix: transaction lookup failed: %w", err)
    }
    if txn == 0 && !c.pendingStart {
        if id := c.conn.IdTag(); id != "" {
            if err := c.conn.RemoteStartTransactionRequest(id); err != nil {
                return fmt.Errorf("ocpp/atronix: remote start failed: %w", err)
            }
            c.pendingStart = true
        }
    } else {
        c.pendingStart = false
    }
    return nil
}

func (c *Atronix) setChargeRate(current float64) error {
    val := strconv.FormatFloat(current, 'f', 0, 64)
    if err := c.cp.ChangeConfigurationRequest("ChargeRate", val); err != nil {
        if strings.Contains(strings.ToLower(err.Error()), "notsupported") {
            c.log.WARN.Printf("ocpp/atronix: ChargeRate not supported, fallback to charging profile (%v)", err)
            return nil
        }
        return fmt.Errorf("ocpp/atronix: ChangeConfiguration ChargeRate=%s failed: %w", val, err)
    }
    c.log.INFO.Printf("ocpp/atronix: ChargeRate=%s A (accepted)", val)
    c.current = current
    return nil
}
```

This extracts the repeated transaction‐lookup, start/stop and vendor checks into small focused functions, flattens the main flow and makes each step self‐contained.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

return c.OCPP.Enable(false)
}

func (c *Atronix) MaxCurrentMillis(current float64) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider refactoring MaxCurrentMillis by extracting logical blocks into helper functions to flatten control flow and reduce repetition.

Here’s a shorter, flatter version of MaxCurrentMillis that pulls each logical block into a helper. This preserves all behavior but removes deep nesting and repeated lookups:

func (c *Atronix) MaxCurrentMillis(current float64) error {
    vendor, model := c.vendorModel()

    if !c.isAtronix(vendor, model) {
        return c.OCPP.MaxCurrentMillis(current)
    }

    if current <= 0 {
        return c.stopTransactionIfAny()
    }

    if err := c.ensureTransactionStarted(); err != nil {
        return err
    }

    if err := c.setChargeRate(current); err != nil {
        return err
    }

    return nil
}

And the new helpers:

func (c *Atronix) isAtronix(vendor, model string) bool {
    return strings.Contains(vendor, "weeyu") || strings.Contains(model, "atronix")
}

func (c *Atronix) stopTransactionIfAny() error {
    txn, err := c.conn.TransactionID()
    if err != nil {
        return fmt.Errorf("ocpp/atronix: transaction lookup failed: %w", err)
    }
    if txn > 0 {
        if err := c.conn.RemoteStopTransactionRequest(txn); err != nil {
            return fmt.Errorf("ocpp/atronix: remote stop failed: %w", err)
        }
    }
    c.pendingStart, c.current, c.enabled = false, 0, false
    return nil
}

func (c *Atronix) ensureTransactionStarted() error {
    txn, err := c.conn.TransactionID()
    if err != nil {
        return fmt.Errorf("ocpp/atronix: transaction lookup failed: %w", err)
    }
    if txn == 0 && !c.pendingStart {
        if id := c.conn.IdTag(); id != "" {
            if err := c.conn.RemoteStartTransactionRequest(id); err != nil {
                return fmt.Errorf("ocpp/atronix: remote start failed: %w", err)
            }
            c.pendingStart = true
        }
    } else {
        c.pendingStart = false
    }
    return nil
}

func (c *Atronix) setChargeRate(current float64) error {
    val := strconv.FormatFloat(current, 'f', 0, 64)
    if err := c.cp.ChangeConfigurationRequest("ChargeRate", val); err != nil {
        if strings.Contains(strings.ToLower(err.Error()), "notsupported") {
            c.log.WARN.Printf("ocpp/atronix: ChargeRate not supported, fallback to charging profile (%v)", err)
            return nil
        }
        return fmt.Errorf("ocpp/atronix: ChangeConfiguration ChargeRate=%s failed: %w", val, err)
    }
    c.log.INFO.Printf("ocpp/atronix: ChargeRate=%s A (accepted)", val)
    c.current = current
    return nil
}

This extracts the repeated transaction‐lookup, start/stop and vendor checks into small focused functions, flattens the main flow and makes each step self‐contained.

@hf81-ble hf81-ble force-pushed the feature/add_atronic_wallbox branch from 86aebe0 to b6b148b Compare November 5, 2025 14:22
@premultiply premultiply marked this pull request as draft November 5, 2025 16:22
@premultiply
Copy link
Member

premultiply commented Nov 5, 2025

I think a new template should suffice here.

The various adjustments to the OCPP code cannot be traced without a log and explanation.
We deliberately do not stop any transactions as this is not compatible with how evcc works.

In this case, it would be advisable to first check with the manufacturer of the box to ensure compatibility with the OCPP standard.

@hf81-ble
Copy link
Author

hf81-ble commented Nov 6, 2025

I think a new template should suffice here.

The various adjustments to the OCPP code cannot be traced without a log and explanation. We deliberately do not stop any transactions as this is not compatible with how evcc works.

For that reason i changed this only in this wallbox implementation. As i have seen, evcc also uses the tag="evcc" on remote Start to the wallbox this should be no issue and i had also not yet any issues while running. But of cause, i surely not have tested all features. But "Überschussladen" with controlling the currents works well.

In this case, it would be advisable to first check with the manufacturer of the box to ensure compatibility with the OCPP standard.

I my case the atronix wallbox will not response to any request from the central system, unless the BootNotification is accepted. This behavior should fully compliant with the OCPP 1.6 standard in 4.2 for BootNotification. If not accepted, it shall also not response. Only if in pending state. But till the CS will not answer the CP, the cp cannot know the state.

So my changes for the OCPP are:

  1. wait for receiving the BootNotification
  2. after that start the Setup Procedure.
    => Setup ist NOT started when only the CONNECTED state is triggered, which means, that only the WebSocket was established but not yet the BootNotification received

I hope i could explain why a added these changes.

@premultiply premultiply self-assigned this Nov 7, 2025
@premultiply
Copy link
Member

Mmmmh.
I couldn't really find anything in the OCPP spec that says a charging station MUST report a BootNotification after establishing a connection.

The way I read the spec, a BootNotification only has to be sent if the station has been restarted – as the name suggests.

Personally, I would find it straightforward if there were such a clear handshake after each connection, but unfortunately, this is not the case in the reality of many implementations.
And, as I said, I cannot find this strict requirement in the specification. Do you have any other/further information and documents?

Apart from that, we already accept every boot notification immediately.
The received content is not relevant to the function and is essentially only logged for diagnostic purposes. The sent response synchronises the CP's clock with the CS.

sourcery-ai[bot]

This comment was marked as off-topic.

sourcery-ai[bot]

This comment was marked as off-topic.

@premultiply premultiply changed the title Feature/add atronic wallbox Attempt to fix EN+ connection issues using OCPP Nov 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants